In http://reviews.llvm.org/D7639#173909, @LegalizeAdulthood wrote:
> In http://reviews.llvm.org/D7639#136297, @xazax.hun wrote: > > > In http://reviews.llvm.org/D7639#136296, @LegalizeAdulthood wrote: > > > > > In http://reviews.llvm.org/D7639#136280, @xazax.hun wrote: > > > > > > > In http://reviews.llvm.org/D7639#136129, @LegalizeAdulthood wrote: > > > > > > > > > If you can show me how to lex a SourceRange without getting that > > > > > error, I'm happy to change it. I tried the method shown in the linked > > > > > diff and I get the same error. > > > > > > > > > > > > As far as I know, the point is that you can not do that without > > > > creating a null terminated string, however you do not need to do that. > > > > What you can do insead: you can lex the whole file buffer starting from > > > > the beginning of a declaration and you can make sure that you finish > > > > lexing on the end of the declaration (e.g.: end lexing when you match > > > > the closing paren of the declaration). This way you might have a bit > > > > more complicated lexing logic but you avoid heap allocation. So the > > > > point is: you construct the lexer using the whole file buffer starting > > > > from the right position and not relying on the source range at all to > > > > finish lexing. > > > > > > > > > Relex the entire file just to avoid a heap allocation? That seems a bit > > > excessive. Do we have any measurements on real code bases that show this > > > to be the better approach? I don't want to optimize like this without > > > real data to show that it is worthwhile. > > > > > > Most of the time this string is going to either be "()" or "(void)". > > > Small string optimization means that there isn't even a heap allocation. > > > > > > Well, you won't relex anything that does not need to be relexed. You only > > initialize the lexer using the whole buffer, so you get a null terminated > > string. But the starting point of the lexing would be the starting source > > location of the declaration you want to lex. And you would only lex the > > tokens you need. I think the lexer is lazy, so initializing with a bigger > > buffer should not cause any performance regressions in case you do not lex > > more tokens that you need. Of course measurements would tell this for sure. > > > I'd like to address this in a subsequent changeset, since ti seems to be a > matter of performance and not a matter of correctness with the existing code > and I'd like to move this check forward. This looks like a very small and quick change, so it would be nice to address this concern in the very first version of the check. ================ Comment at: clang-tidy/readability/RedundantVoidArgCheck.cpp:244 @@ +243,3 @@ + removeVoidArgumentTokens( + Result, shrinkRangeByOne(SourceRange(CStyleCast->getLParenLoc(), + CStyleCast->getRParenLoc())), ---------------- Character-based offsets can be brittle in case escaped newlines and alternative tokens are used. I'd prefer getting the type range directly when it's possible. Here and for other casts something like this should work: `CStyleCast->getTypeInfoAsWritten()->getTypeLoc().getSourceRange()`. Also, do we need separate functions for `ExplicitCastExpr` and two of its children classes? ================ Comment at: test/clang-tidy/readability-redundant-void-arg.c:77 @@ +76,3 @@ + +// intentionally not LLVM style to check preservation of whitespace +typedef void (function_ptr2) ---------------- Special tests for whitespace handling seem to be superfluous when you just need to check that no changes are made. Also, the invariant "no warnings => no fixes" seems to be safe to rely on, so you can remove all CHECK-FIXES and add one "CHECK-MESSAGES-NOT: warning:" so that the script runs the FileCheck on the output. http://reviews.llvm.org/D7639 EMAIL PREFERENCES http://reviews.llvm.org/settings/panel/emailpreferences/ _______________________________________________ cfe-commits mailing list cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
