Bug #2067

C4927 warnings in Core with latest MSVC

Added by Zbigniew Rebacz almost 2 years ago. Updated almost 2 years ago.

Status:ApprovedStart date:09/13/2020
Priority:HighDue date:
Assignee:Miroslav Fidler% Done:

0%

Category:CoreSpent time:-
Target version:Release 2020.2

Description

It seems that compiling Core with latest MSVC produces a lot of warnings that have C4927. Screenshot with warnings attached.

Would be nice to fix for 2020.1.

C4927.png (178 KB) Zbigniew Rebacz, 09/13/2020 01:04 PM

StringBufferPatch.diff Magnifier (23.8 KB) Zbigniew Rebacz, 09/22/2020 07:21 PM

Skylark.diff Magnifier (671 Bytes) Zbigniew Rebacz, 09/23/2020 09:06 PM

History

#1 Updated by Miroslav Fidler almost 2 years ago

Yeah, that is because MSVC is broken. Lucky that we have clang...

Mirek

#2 Updated by Miroslav Fidler almost 2 years ago

  • Assignee changed from Miroslav Fidler to Zbigniew Rebacz

Have you managed to fix this?

#3 Updated by Zbigniew Rebacz almost 2 years ago

  • Assignee changed from Zbigniew Rebacz to Miroslav Fidler

As we discuss we will keep it like it is and our recommendation is to do not use latest MSVC versions. If it will not be fixed in reasonable amount of time, we will remove all MSVC related code from TheIDE and we will base only open source solution such as clang ;) Open source compilers can be modify, so bugs like this can be fix easily ;)

Now to be serious. Maybe we should add explicit keyword to char* conversion operator of StringBuffer:

    explicit operator char*()       { return Begin(); }

This will make this warnings errors... Explicit casting will solve the issue in all cases:
static_cast<String>(sb);

errors are fixed.

You should returhing this whole situation and maybe we should apply explicit keywords in our code to avoid such situation.

Reference:
- https://google.github.io/styleguide/cppguide.html#Implicit_Conversions

Maybe from the usability point of view this change is heave, because each time you need to type casting operator. However from the code security point of view it is improvement.

#4 Updated by Miroslav Fidler almost 2 years ago

  • Status changed from New to In Progress
  • Assignee changed from Miroslav Fidler to Zbigniew Rebacz

Well, that is not very helpful. I hoped that you fix it with String(r) solution and send me the diff or whatever so that I can decide whether it is worth it.

As for that explicit nonsense, do you plan to fix C++ std:: library too? What about boost?

#5 Updated by Zbigniew Rebacz almost 2 years ago

I will try to generate patch in the weekend.

#7 Updated by Miroslav Fidler almost 2 years ago

  • Status changed from In Progress to Ready for QA

Thank you. I have decided to go with it, so please check that trunk now works... (if it does, close the task).

#8 Updated by Zbigniew Rebacz almost 2 years ago

  • Status changed from Ready for QA to Approved

#9 Updated by Zbigniew Rebacz almost 2 years ago

  • File Skylark.diffMagnifier added
  • Status changed from Approved to In Progress
  • Assignee changed from Zbigniew Rebacz to Miroslav Fidler

Two warnings in Skylark package would be good to fix...

#10 Updated by Zbigniew Rebacz almost 2 years ago

  • Status changed from In Progress to Approved

Also available in: Atom PDF