[PATCH] D37014: [clang-tidy] Add a checker to remove useless intermediate variables before return statements with comparisons

[Tristan Bourvon via Phabricator via cfe-commits]

tbourvon added a comment.

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? 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 */
  
  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?


Repository:
  rL LLVM

https://reviews.llvm.org/D37014