[PATCH] D56611: [clangd] A code action to swap branches of an if statement

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.


================
Comment at: clang-tools-extra/clangd/SourceCode.h:63
 
+/// Turns a token range into a half-open range and checks its correctness.
+/// The resulting range will have only valid source location on both sides, both
----------------
sammccall wrote:
> I think the semantics of the locations (expansion vs spelling) need to be spelled out here, at least in the comment.
Added a small comment, but it could probably be improved. Let me know what's missing.


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/SwapIfBranches.cpp:60
+        Ctx.getSourceManager(), Ctx.getLangOpts(),
+        SourceRange(If->getIfLoc(), If->getCond() ? If->getCond()->getEndLoc()
+                                                  : If->getIfLoc()));
----------------
sammccall wrote:
> what's the case where the condition is null?
I initially thought of invalid parse trees (i.e. `if() {}`), but it turns out this never happens. 

>From experience, it's better to assume everything can be null in the AST. This saves a lot of time chasing rare null dereferences later and a lot of time investigating whether something is null in a particular case we're facing.

In our particular case `IfStmt` seems to always have a condition, but the condition will have an invalid location when it's empty. I've added a test that the action still works (I see no reason why it shouldn't if we assume the parser recovery is good). Happy to discuss and adjust, though, let me know what you think.


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/SwapIfBranches.cpp:63
+    if (R && (halfOpenRangeContains(Ctx.getSourceManager(), *R, CursorLoc) ||
+              R->getEnd() == CursorLoc)) {
+      Result = If;
----------------
sammccall wrote:
> the explicit checks for equality at the endpoint seem... odd
> do you need a halfOpenRangeTouches() or so?
Done. Thanks for the suggestion, this does make the code much clearer.


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/SwapIfBranches.cpp:101
+
+  auto ThenRng = toHalfOpenFileRange(SrcMgr, Ctx.getLangOpts(),
+                                     If->getThen()->getSourceRange());
----------------
sammccall wrote:
> do this in apply()?
> 
> This is an example refactoring, if we have to add a bunch of checks because it fires too often then we should try to solve that systematically.
Done.

This goes back to a similar comment about precomputing the replacements... Doing this in advance means we're not showing the check in some cases where it would later fail because of macros.

However, macros support is poor in the current state of the checks anyway, we'll need to invest some more time to make it better.


================
Comment at: clang-tools-extra/unittests/clangd/TweakTests.cpp:116
+TEST_F(TweakTest, SwapIfBranches) {
+  llvm::StringLiteral ID = "SwapIfBranches";
+
----------------
sammccall wrote:
> why StringLiteral here?
Seems like a good default for a constant string, i.e. it can compute the size at compile time.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D56611