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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
