ioeric created this revision.
ioeric added reviewers: klimek, djasper.
ioeric added subscribers: alexshap, hokein, omtcyfz, cfe-commits.
Herald added a subscriber: klimek.
https://reviews.llvm.org/D24383
Files:
include/clang/Tooling/Core/Replacement.h
lib/Tooling/Core/Replacement.cpp
unitte
djasper added a comment.
This seems like a pretty weird function to have in the interface. Could you
explain what use case(s) you are envisioning?
https://reviews.llvm.org/D24383
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.
alexshap added a comment.
disclaimer: i don't know if this method is the right thing to add (to be honest
i would prefer a better interface but don't have any good suggestions on my
mind at the moment), i see several issues (IMO, i might be mistaken, apologize
in advance) with the current inter
This function has been implemented in several places, so I figure it might
be useful to have this function in the library. Although I'm not sure
whether this should be a class interface or just a standalone helper
function.
@alexshap: thanks for the explanation. For your point B, I'm not sure
abo
There are several things I find particularly confusing about this.
Fundamentally, Replacements refer to a specific version of the code. Say
you have a Replacements object R and a Replacement r that you want to
integrate into it. Further assume, that C0 is the initial version of the
code whereas C1
@ioeric:
1.
regarding B:
i apologize in advance, i am not sure if it's directly related to your
patch, but i'm kinda afraid that the current approach will shadow
the issue (if it's considered to be an issue).
For simplicity let's take a look at the following example:
Point.h:
struct Point {};
ioeric added a comment.
@djasper Thanks for the insight into the problem.
For the cases you listed, I have added some test cases. Case 1-3 can be handled
as expected. And you are right, the behavior for case 4 is interesting.
But I still agree that `addOrMerge` is confusing. As you said, the in
ioeric updated this revision to Diff 71006.
ioeric added a comment.
- Also cleanup comments around redundant colons/commas in format::cleanup.
https://reviews.llvm.org/D24383
Files:
include/clang/Tooling/Core/Replacement.h
lib/Tooling/Core/Replacement.cpp
unittests/Tooling/RefactoringTest
Somehow the commit message was wrong...should be "Added more test cases"
instead.
On Mon, Sep 12, 2016 at 4:38 PM Eric Liu wrote:
> ioeric updated this revision to Diff 71006.
> ioeric added a comment.
>
> - Also cleanup comments around redundant colons/commas in format::cleanup.
>
>
> https://r
djasper added a comment.
Ok.. Thought some more and discussed with Manuel.
I think we should do a partial solution for now. I still think addOrMerge is
harmful and it is always never the right thing to use. The codepaths that
currently implement some version of it, should likely reconsider.
Wh
ioeric added a comment.
In https://reviews.llvm.org/D24383#539942, @djasper wrote:
> Ok.. Thought some more and discussed with Manuel.
>
> I think we should do a partial solution for now. I still think addOrMerge is
> harmful and it is always never the right thing to use. The codepaths that
> c
djasper added a comment.
The problem is that it is implicit behavior, that's not easy to understand.
What's worse is that the behavior of add and merge would fundamentally be
reverse. If you add two inserts at the same location, the first one would come
first. If you merge them, the second one
ioeric added a comment.
In https://reviews.llvm.org/D24383#540296, @djasper wrote:
> The problem is that it is implicit behavior, that's not easy to understand.
> What's worse is that the behavior of add and merge would fundamentally be
> reverse. If you add two inserts at the same location, th
ioeric abandoned this revision.
ioeric added a comment.
Abandon in favor of https://reviews.llvm.org/D24515
https://reviews.llvm.org/D24383
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-co
14 matches
Mail list logo
