[PATCH] D91485: [clang-tidy] ElseAfterReturn check wont suggest fixes if preprocessor branches are involved

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.cpp:118
 static void removeElseAndBrackets(DiagnosticBuilder &Diag, ASTContext &Context,
-                           const Stmt *Else, SourceLocation ElseLoc) {
+                                  const Stmt *Else, SourceLocation ElseLoc) {
   auto Remap = [&](SourceLocation Loc) {
----------------
Unrelated formatting change?


================
Comment at: clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.cpp:189
+static bool hasPreprocessorBranchEndBetweenLocations(
+    ElseAfterReturnCheck::ConditionalBranchMap ConditionalBranchMap,
+    const SourceManager &SM, SourceLocation StartLoc, SourceLocation EndLoc) {
----------------
Should the map be passed by const ref to avoid copies?


================
Comment at: clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.cpp:203
+
+  auto &ConditionalBranches =
+      ConditionalBranchMap[SM.getFileID(ExpandedEndLoc)];
----------------
Should this be `const auto &` to be clear there are no modifications to the map?


================
Comment at: clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.cpp:214
+
+  // First conditional block that ends after ExpandedStartLoc
+  const auto *Begin =
----------------
Missing a full stop at the end of the comment.


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/readability-else-after-return.cpp:267
+
+  // These cases, Same as above but with an #else block
+  if (true) {
----------------
Same -> same
Also, missing a full stop.


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/readability-else-after-return.cpp:285
+
+// ensure it can handle macros
+#define RETURN return
----------------
ensure -> Ensure
Also, missing a full stop.


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/readability-else-after-return.cpp:304
+
+  // Whole statement is in a conditional block so diagnost as normal.
+#if 1
----------------
diagnost -> diagnose


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/readability-else-after-return.cpp:313
+#endif
+}
----------------
We should probably add some tests for more pathological cases, like:
```
#if 1
if (true) {
  return;
#else
{
#endif
} else {
  return;
}
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91485