[PATCH] D51949: [WIP][clang-tidy] initial ideas to isolate variable declarations

[Jonas Toth via Phabricator via cfe-commits]

JonasToth marked 11 inline comments as done.
JonasToth added inline comments.


================
Comment at: clang-tidy/readability/IsolateDeclCheck.cpp:68
+    if (Start.isInvalid() || Start.isMacroID())
+      return SourceLocation();
+  }
----------------
kbobyrev wrote:
> Also, I don't think this should happen with the correct behavior. `llvm::Expected<T>` looks like a better alternative for error handling here.
It currently happens for `typedef int * IntPtr; IntPtr p;`.  Regarding `Expected<T>` see other comment.


================
Comment at: clang-tidy/readability/IsolateDeclCheck.cpp:278
+  if (!PotentialSnippets)
+    return;
+
----------------
kbobyrev wrote:
> Both for `PotentialSlices` and `PotentialSnippets`: is there any case where any can not be determined and this is not an error? They both are `llvm::Optional<T>`s and the `check(Result)` just returns if any of them are not set, is there any case when any of the variables is actually not set, but this is the correct behavior? If not, IMO it would be better to use `llvm::Expected`: then, if the check fails, either print `.takeError()` message or propagate it "upstairs" (although I'm not really sure what would be better here).
Currently the `Optional` and checking for invalid and so on everywhere is result of the WIP and the bugs I encountered and try to fix. In general the operations, like re-lexing can fail judging from the interface, same for retrieving SourceLocations, so I took the very conservative approach to check everything.
In my opinion it is more user-friendly to just emit the warning, but without transformation instead of an error that the transformation fails for some reason. Running this check over multiple bigger code-bases will give better data to make an actual judgement on this.

In general there are code-constructs where transformation _could_ be dangerous or unexpted, all I can think of includes macros.

```
int SomeConfig = 42,
#if BUILD_WITH_X
     AnotherConfig = 43;
#else 
    AnotherConfig = 44;
#endif
```

This example is not unreasonable and currently transformed incorrectly. I wanted to have an mechanism to just bail if something is fishy. I tried `llvm::Expected` but thought that using `llvm::Optional` was just simpler for prototyping.
Semantically `Expected` would fit better, because the transformation is actually expected to work. Emitting notes that contain information from the errors might be user-friendly, but having a `note: Can not automatically transform this declaration statement` is probably similarly  informative.
For now I'd stick with `Optional` and it might be even the better fit in general, considering that more declaration kinds should be implemented in the future (similar to the other, currently staled check).


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D51949