ilya-biryukov added a comment.
In D64518#1588092 <https://reviews.llvm.org/D64518#1588092>, @ymandel wrote:
> This seems like a good candidate for configuration -- the user could then
> choose which mode to run in. But, I'm also open to just reporting these
> conditions as errors. It's already in a context that returns Expected, so
> its no trouble; it's just a matter of choosing what we think is "correct".
WRT to returning `Expected` vs `Optional`. Either seems fine and in the spirit
of the library, depending on whether we want to produce more detailed errors.
However, if we choose `Optional` let's stick to it, as practice shows switching
from `Optional` to `Expected` correctly is almost impossible, as that requires
a lot of attention to make sure all clients consume the errors (and given that
it's an error case, tests often don't catch unconsumed errors).
I would personally go with `Optional` here (meaning the client code would have
to say something generic like `could not map from macro expansion to source
code`). But up to you, not a strong preference.
WRT to which cases we choose to handle, I'd start with a minimal number of
supported examples (covering full macro expansion, or inside a single argument)
and gradually add other cases as we find use-cases. What are your thoughts on
that?
================
Comment at: clang/lib/Tooling/Refactoring/Transformer.cpp:76
+static llvm::Optional<CharSourceRange>
+makeValidRange(CharSourceRange Range, const MatchResult &Result) {
+ const SourceManager &SM = *Result.SourceManager;
----------------
ymandel wrote:
> ilya-biryukov wrote:
> > Could we add unit tests for this particular function?
> >
> > Interesting cases (`[[` and `]]` mark the start and end of a range):
> > ```
> > #define FOO(a) a+a;
> > #define BAR 10+
> >
> > // change part of a macro argument
> > int a = FOO([[10]] + 10);
> >
> > // change the whole macro expansion
> > int b = [[FOO(10+10)]];
> >
> > // Try to change 10 inside 'BAR', but not '+'.
> > // Should this fail? Should we give a warning?
> > int c = BAR 3;
> >
> > // Try changing the lhs (10) of a binary expr, but not rhs.
> > // Is that allowed? Should we give a warning?
> > int d = FOO(10);
> > ```
> Sure. What do you think of exposing this function in
> clang/include/clang/Tooling/Refactoring/SourceCode.h and testing it from
> there?
Sounds reasonable. Was thinking of a better name, maybe something like
`getRangeForEdit()`?
Would also suggest to accept `SourceManager` and `LangOptions` instead of
`MatchResult` to narrow down the requirements on the clients.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D64518/new/
https://reviews.llvm.org/D64518
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
