dkrupp added a comment. In D55125#1315335 <https://reviews.llvm.org/D55125#1315335>, @Szelethus wrote:
> I see your point, but here's why I think it isn't a bug: I like to see macros > as `constexpr` variables, and if I used those instead, I personally wouldn't > like to get a warning just because they have the same value. In C, silencing > such a warning isn't even really possible. > > Another interesting thought, @donat.nagy's check works by comparing actual > nodes of the AST, while this one would work with `Lexer`, but they almost > want to do the same thing, the only difference is that the first checks > whether two pieces of code are equivalent, and the second checks whether they > are the same. Maybe it'd be worth to extract the logic into a simple > `areStmtsEqual(const Stmt *S1, const Stmt *S2, bool ShouldCompareLexically = > false)` function, that would do AST based comparison if the last param is set > to false, and would use `Lexer` if set to true. After that, we could just add > command line options to both of these checks, to leave it up to the user to > choose in between them. Maybe folks who suffer from heavily macro-infested > code would rather bear the obvious performance hit `Lexer` causes for little > more precise warnings, and (for example) users of C++11 (because there are > few excuses not to prefer `constexpr` there) could run in AST mode. > > edit: I'm not actually all that sure about the performance hit. Just a guess. > > But I'm just thinking aloud really. Wiring out the lexical comparison and AST based comparison sounds like an interesting idea, however IMHO such a setting is too coarse-grained on the file level. My guess would be that it depends from expression (fragment) to expression fragment how you want to compare them: for macros lexical comparison is better, for arithmetic expressions with many parentheses AST based recursive comparison may be a better fit (as implemented also in this checker). CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55125/new/ https://reviews.llvm.org/D55125 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
