firolino added inline comments.
================
Comment at: clang-tidy/readability/OneNamePerDeclarationCheck.cpp:153
+ const SourceRange FVLoc(DeclStmt->getLocStart(), Location);
+ std::string UserWrittenType =
+ Lexer::getSourceText(CharSourceRange::getCharRange(FVLoc), SM,
----------------
alexfh wrote:
> I wonder whether re-lexing the whole range and working on tokens instead
> would be a better and more robust approach? You wouldn't have to deal with
> whitespace and comments then. What do you think?
Sounds interesting! I am gonna give this a try.
================
Comment at: clang-tidy/readability/OneNamePerDeclarationCheck.cpp:163
+ // const int *& -> const int
+ // long ** -> long int
+ size_t Pos = std::string::npos;
----------------
alexfh wrote:
> Why do we want `long int` and not `long`?
Sorry, misleading comment.
================
Comment at: clang-tidy/readability/OneNamePerDeclarationCheck.h:25
+class OneNamePerDeclarationCheck : public ClangTidyCheck {
+private:
+ std::string getUserWrittenType(const DeclStmt *DeclStmt, SourceManager &SM);
----------------
alexfh wrote:
> nit: move the private section to the end.
[[ http://llvm.org/docs/CodingStandards.html#use-of-class-and-struct-keywords |
LLVM Coding Standards ]] is putting the private section at the beginning as
well. Even in the code base, it seems to be the general rule. While you are
reading code from top to bottom, you need to get the necessary context
(variables and their types) first, before proceeding to the functional code.
Otherwise, you would have to scroll all the time down to the private variables
- when passing its usage in a function - to get its type. This would impair the
reading fluency and thus the straight-forwarding understanding of the code.
================
Comment at: clang-tidy/utils/LexerUtils.cpp:41-56
+ const auto &SM = Context.getSourceManager();
+ const auto &LO = Context.getLangOpts();
+ auto StartOfFile = SM.getLocForStartOfFile(SM.getFileID(Location));
+
+ Token CurrentToken;
+ CurrentToken.setKind(tok::unknown);
+
----------------
alexfh wrote:
> Is it significantly more efficient than
>
> Token Tok;
> do {
> Tok = getPreviousNonCommentToken(Context, Location);
> } while (!Tok.isOneOf(tok::unknown, TokenToFind));
> return Tok.getLocation();
>
> ?
>
> If it's not, then this function might be not so much useful to be here. At
> least, its interface is rather specific to the use case you have. Why, for
> example, it returns a location instead of the token itself?
More efficient? In worst-case (token not found) yes, in my use-case probably
not that significant. You are right, changing the interface to `Token
findTokenBackward(...)` seems to be more generic than returning the location
directly.
https://reviews.llvm.org/D27621
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
