tbourvon added a comment. In https://reviews.llvm.org/D37014#850088, @lebedev.ri wrote:
> 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. IMO it makes more sense to expose `PreprocessingRecord`, as registering callbacks and maintaining a private inner database for every check not only isn't ideal in terms of performance, but also duplicates logic and could lead to inconsistencies. I think I'm going to work on getting this object accessible through `MatchResult` and `MatchFinder` so that it is accessible from both checks and matchers. Probably in a separate patch. Please let me know if you have any more thoughts on this. 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
