fix: re-enable warnings as errors by default and fix the warnings #11854
Conversation
| */ | ||
| virtual Token *getNextToken() = 0; | ||
| virtual void setParser(void* parser) {}; | ||
| virtual void setParser(void* parser) {} |
There was a problem hiding this comment.
This is generated code, so should not be done. An PR (javacc/javacc#315) has been created upstream.
There was a problem hiding this comment.
Ok, I can see that it gets generated by the upstream javacc
however if this is not found then it falls back to using the existing files
message(STATUS " Fall back to JavaCC not installed, using existing files.")
and since this existing file is part of the code base and it causes a warning, which the purpose of this PR is to remove all warnings and turn on warnings as errors, What's wrong with updating the existing file.
When the javacc is updated upstream and the generated code matches, then the later pull request will address this issue again, but until then, why not remove the existing warnings from the code which is not always autogenerated as shown above?
There was a problem hiding this comment.
The javaCC code is run each time when the one of the *.jj or one of the non generated included files is changed and thus at that moment the ; would reappear (and would flow into the "using existing files").
As written there is a proposed PR for this upstream (javacc/javacc#315) but I'm not confident that this will be integrated soon and afterwards be available in a release of javaCC (and it is also unclear what kind of problems we might find there at that moment).
| /w14906 # string literal cast to 'LPWSTR' | ||
| /w14928 # illegal copy-initialization; more than one user-defined | ||
| # conversion has been implicitly applied | ||
| /WX # Treat warnings as errors |
There was a problem hiding this comment.
When not mistaken this option for the different platforms will stop at the first error and this was not desirable
There was a problem hiding this comment.
Why is this not desirable?
Surely we want a warning free codebase?
There was a problem hiding this comment.
Indeed a warning free codebase is very desirable but some parts are outside of our control (like e.g the ; in the generated code) and there are more of those places. This would mean that the compiler would always stop at these places and it would be a big burden on / impossibility for projects that regularly build doxygen (e.g. CGAL).
There was a problem hiding this comment.
right in that case do we just shelve this PR as the issues are not internally resolvable?
instead I will just remove the other warnings from the two files I have changed in this PR without comment. But raise that as a separate PR addressing just those concerns and keeping the warnings as errors and the code dependant on upstream javacc out of it
turn on warnings as errors and fix the existing warnings