#1059 Reduce compile warnings (patch)

Completed
closed
nobody
Scintilla (356)
5
2014-08-21
2014-07-12
Alpha
No

The attached patch reduces some -Wshadow and -Wswitch-default warnings. It only touches locations where I felt the author's intent was apparent.

1 Attachments

Related

Feature Requests: #1063

Discussion

  • Neil Hodgson
    Neil Hodgson
    2014-07-12

    It is far too easy to introduce bugs when the code changed isn't something that you yourself are using so will not see any problems. I avoid fixing minor problems in most lexers because of this. Which of these lexers do you use?

     
  • Neil Hodgson
    Neil Hodgson
    2014-07-12

    Adding default cases to the switches changes the behaviour of those switches: previously ignored values are now going to be processed as if they were whichever case the default was added to.

     
  • Alpha
    Alpha
    2014-07-12

    I use CMake, Conf, Inno, LaTeX, MySQL, and Nsis, though admittedly, they are not main languages for me.
    The only places I added a default case are lexers where if the current implicit default were ever taken, it would result in the remainder of the file being styled the same color, because these lexers only change state within the switch. No, I have not been able to force execution down that code path, but stray solar radiation could.
    When I read through Scintilla's source, the numerous places where shadowed variables are used greatly increase my difficulty with following the logic. I left spots alone where it looked ambiguous if the author intended to create a new variable, or wanted to actually use the shadowed one.

    In the end, none of these changes are critical. However, I believe they enhance readability and maintainability of the affected sections of source. I understand, though, if you prefer to pick and choose a few of the changes, or none at all, to commit.

     
  • Neil Hodgson
    Neil Hodgson
    2014-07-13

    If the remainder of the file is coloured in one style then that is an obvious sign that something is not being handled which can then be fixed.

    If you just want to get rid of the warning then do so without a functional change with "default: break;" or possibly "default: assert(false); break;" so that there will be a nice clean error if the unexpected occurs when debugging.

    According to -Wshadow, there are 7 occurrences of shadowed variables, all in lexers and only one in a lexer you use which is quite obvious and limited in scope. The AU3 and PowerPro occurrences are also simple - just trust the language scope rules. The VHDL and Rust occurrences are messier and may be worth fixing but that should be done by raising an issue and leaving it to someone that uses and understands those languages to fix.

     
  • Neil Hodgson
    Neil Hodgson
    2014-07-24

    [f8ff45] Disables -Wshadow in LexerModule.h which ensures this won't be reported for lexers.

     

    Related

    Commit: [f8ff45]

  • Re: [f8ff45] I wouldn't do that as it just hides issues that are worth fixing at least for readability purposes, and may actually show a bug when developing a lexer. If one is really bothered by these warnings one can simply not enable/disable the flag on the build command.

    Also, note that GCC's diagnostic [error|ignored|warning] pragmas were introduced by GCC 4.2 AFAIK (4.2.4 if I trust http://dbp-consulting.com/tutorials/SuppressingGCCWarnings.html), and may lead to warnings on earlier versions, so the preprocessor conditional should rather be something like if defined(__GNUC__) && (__GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINIOR__ >= 2)) (or better to check for 4.2.4 specifically).

     

    Related

    Commit: [f8ff45]

  • Neil Hodgson
    Neil Hodgson
    2014-07-24

    The problem with not having the pragma is that it leads to issues like this one being created. Those that want to see shadow warnings can change LexerModule.h while working on files affected by it. It may be better to place the pragma in each affected file.

    GCC versions earlier than 4.3 are now rare as 4.3 was released in March 2008. Even Apple have stopped distributing GCC 4.2.

     
  • Alpha
    Alpha
    2014-07-27

    Sorry I have not been able to come back to this recently.

    I also believe that using a pragma to hide warnings (warnings that can be fixed in code, not warnings generated by broken compilers) is undesirable. I like being able to easily control what warnings I see via compiler flags.

    Re: switch behaviour changes, I will write up an assert based patch for it soon.

     
    • Neil Hodgson
      Neil Hodgson
      2014-07-27

      With further thought on the switch default issue, I do not see switches without defaults as a problem. I will not accept patches that simply add default labels to switches.

      If you want to move the pragmas for shadowing to narrow down their scope then that is fine.

       
  • Neil Hodgson
    Neil Hodgson
    2014-08-14

    • status: open --> closed