[PATCH] D74214: [clang-tidy] Fix PR#34798 'readability-braces-around-statements breaks statements containing braces.'

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/readability/BracesAroundStatementsCheck.cpp:21
 
-tok::TokenKind getTokenKind(SourceLocation Loc, const SourceManager &SM,
-                            const ASTContext *Context) {
-  Token Tok;
+bool tryGetTokenKind(SourceLocation Loc, const SourceManager &SM,
+                     const ASTContext *Context, tok::TokenKind &Kind) {
----------------
Could you change this to `llvm::Expected<Kind> tryGetTokenKind(SourceLocation Loc, const SourceManager &SM, const ASTContext *Context)`. From a readability and performance standpoint, returned parameters are generally better than out parameters.


================
Comment at: clang-tools-extra/clang-tidy/readability/BracesAroundStatementsCheck.cpp:76-77
+
+  // We need to check that it is not 'InitListExpr' which ends with 
+  // the tokens '};' because it will break the following analysis
+  tok::TokenKind NextTokKind;
----------------
aaron.ballman wrote:
> Is there evidence that this behavior is desired? I have a hunch that this is a bug in Clang -- not all `InitListExpr`s will terminate with a semicolon, such as ones that appear as function arguments, like `foo({1, 2, 3});`, so I'm surprised to see it included here.
On a related note, are you sure the cause of this issue is `makeFileCharRange`? AFAIK, that does not involve any examination of tokens. It purely (attempts) to map from potentially a mix of expanded locations and file locations to purely file locations (and in the same file for that matter).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74214/new/

https://reviews.llvm.org/D74214