alexfh added inline comments.
================
Comment at: clang-tidy/readability/OperatorsRepresentationCheck.cpp:34
+
+ if (const auto *B = Result.Nodes.getNodeAs<BinaryOperator>("binary")) {
+ switch (B->getOpcode()) {
----------------
aaron.ballman wrote:
> mgehre wrote:
> > aaron.ballman wrote:
> > > alexfh wrote:
> > > > aaron.ballman wrote:
> > > > > I think this would make more sense lifted into an AST matcher --
> > > > > there are bound to be a *lot* of binary operators, so letting the
> > > > > matcher memoize things is likely to give better performance.
> > > > Any reasons not to do this on the lexer level?
> > > Technical reasons? None.
> > > User-experience reasons? We wouldn't want this to be on by default (I
> > > don't think) and we usually don't implement off-by-default diagnostics in
> > > Clang. I think a case could be made for doing it in the Lexer if the
> > > performance is really that bad with a clang-tidy check and we couldn't
> > > improve it here, though.
> > Do I correctly understand that "doing this on lexer level" would mean to
> > implement this as a warning directly into clang? If yes, would it be
> > possible to generate fixits and have them possibly applied automatically
> > (as it is the case for clang-tidy)?
> You are correct, that means implementing it as a warning in Clang. I believe
> you can still generate those fixits from lexer-level diagnostics, but have
> not verified it.
>
> However, I don't think this diagnostic would be appropriate for Clang because
> it would have to be off by default.
Actually, I was thinking about changing this clang-tidy check to analyze token
stream somehow (probably by handling `PPCallbacks` to detect ranges that need
to be re-lexed) instead of matching the AST. I didn't intend to propose a new
Clang warning (but it looks like the wording was misleading).
https://reviews.llvm.org/D31308
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
