lebedev.ri added a comment. In https://reviews.llvm.org/D37014#850064, @tbourvon wrote:
> In https://reviews.llvm.org/D37014#849157, @lebedev.ri wrote: > > > Please add the following test: (and make sure that it does the right thing > > :)) > > > > bool f_with_preproc_condition() { > > auto test = 42; > > assert(test == 42); > > return test; > > } > > > > > > I.e. if `-DNDEBUG` is present, variable is not needed, but if `-DNDEBUG` is > > *NOT* present... > > > Shouldn't we ignore these cases whether or not `-DNDEBUG` is present? That's *exactly* what i was talking about. > If we apply the fix here and remove the variable while we have `-DNDEBUG`, > and then remove this define, then we'll get a compiler error for `test` not > found. > In this case it can be fixed fairly easily, but this could in fact introduce > bugs with more complex conditional macros and preprocessor directives. For > example: > > #ifdef WIN32 > #define MY_MACRO(test) test = 0 > #else > #define MY_MACRO(test) /* nothing */ > #endif > > bool f() { > auto test = 42; > MY_MACRO(test); > return (test == 42); > } > > > If we want to ignore these cases not matter what, which seems to me like the > most logical and reasonable thing to do, we need to be able to know if there > are preprocessor directives between the variable declaration and the `return` > statement. > In order to do that, we would need to be able to get the > `PreprocessingRecord` of the current compilation in order to call > getPreprocessedEntitiesInRange() > <https://clang.llvm.org/doxygen/classclang_1_1PreprocessingRecord.html#a1f5d06d192dad434958ff00975aaaddd>, > but AFAICT this cannot be accessed from clang-tidy checks at the moment. > Should we introduce a way to reach that object from checks? Currently, there is a `registerPPCallbacks()` function in `ClangTidyCheck` class, which allows you to register a subclass of `PPCallbacks`, and manually handle all the preprocessor thingies. That being said, getPreprocessedEntitiesInRange() <https://clang.llvm.org/doxygen/classclang_1_1PreprocessingRecord.html#a1f5d06d192dad434958ff00975aaaddd> looks interesting, and it *might* help me in https://reviews.llvm.org/D36836. Repository: rL LLVM https://reviews.llvm.org/D37014 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
