Preventing several replacements on a macro call.

[Argyrios Kyrtzidis via cfe-commits]

> On Sep 10, 2015, at 12:22 PM, Angel Garcia via cfe-commits <cfe-commits at lists.llvm.org> wrote:
> 
> Hi Argyrios,
> 
> Thank you for your answer. I think that it is pretty clear that we don't really want to allow changes like the one in your example. But my problem is almost the opposite. I have something like this:
> 
> #define MY_MAC(x,y) doSomething(x, y);
> 
> which is used in this way:
> 
> MY_MAC(var1, var1);
> 
> and I want to replace all usages of "var1" to another thing, "var2". Each of the arguments of the macro is used only once, so it should be safe to replace each of them independently and obtain
> 
> MY_MAC(var2, var2);
> 
> But right now what I would obtain is
> 
> MY_MAC(var2, );

That doesn’t seem like a case where the change was rejected but where it was misapplied.
Could you provide a reduced test case along with your clang-tidy changes and how to invoke it against the test case ?

> 
> They look like different cases to me, even though the threshold is not very clear. Maybe there is a way to allow this transformation, but still disallow yours. Or do you think that this isn't safe? I started working on this hardly a month ago, so I am not totally aware of the risks of these things.
> 
> Angel
> 
> 
> 
> On Thu, Sep 10, 2015 at 9:01 PM, Argyrios Kyrtzidis <kyrtzidis at apple.com <mailto:kyrtzidis at apple.com>> wrote:
> Hi Angel,
> 
> This part of the code is conservative because it is not clear if accepting the change in all the different places where the macro argument is expanded is acceptable or not.
> For a contrived example, let’s say you have this macro definition:
> 
> 	#define MY_MAC(x) foo(x) + bar(x)
> 
> and you use it like this:
> 
> 	int a = MY_MAC(b);
> 
> which expands to this:
> 
> 	int a = foo(b) + bar(b);
> 
> Now suppose you want to find all places where “foo(b)” is used and change it to “foo(b.c)”. You walk the AST and find “foo(b)” from the macro expansion and you change ‘b’ to ‘b.c’. This change will apply to the macro argument:
> 
> 	int a = MY_MAC(b.c);
> 
> But this now expands like this:
> 
> 	int a = foo(b.c) + bar(b.c)
> 
> And you unintentionally also changed the ‘bar’ call.
> 
> 
> Now, ideally we would keep track of all such changes and if you tried to change the ‘b’ in both “foo(b)” and “bar(b)” we would automatically accept it; but this is a bit complicated.
> In the meantime we could add a ‘knob' to control this behavior. We could have a field in Commit object that you set to true to indicate that it is ok to accept a change in a macro argument that expands in multiple places, and also for convenience add such a knob to EditedSource object for accepting in all commits without needing to set it for each commit separately.
> 
> What do you think ?
> 
>> On Sep 9, 2015, at 6:05 AM, Angel Garcia via cfe-commits <cfe-commits at lists.llvm.org <mailto:cfe-commits at lists.llvm.org>> wrote:
>> 
>> +cfe-commits
>> 
>> On Tue, Sep 8, 2015 at 6:56 PM, Angel Garcia <angelgarcia at google.com <mailto:angelgarcia at google.com>> wrote:
>> Hi Ted,
>> 
>> I was working on a clang-tidy check, and today I discovered that it was unable to do several replacements in different arguments of the same macro call. At first, I thought it was a bug, and trying to figure out why this was happening, I found that the replacements were rejected in lib/Edit/EditedSource.cpp:46, where there is a comment that says "Trying to write in a macro argument input that has already been written for another argument of the same macro". This comment means that this changes are rejected on purpose.
>> 
>> At the commit history, I saw that you had commited <http://reviews.llvm.org/rL152141> this code (that's why I am asking you). Do you think that there is a way around this? I don't really understand why there is a particular case for the macros here, and understanding it would help me to decide whether I should give up on trying to make this work, or try to find a "better" solution.
>> 
>> Thanks and sorry for the inconvenience,
>> Angel
>> 
>> _______________________________________________
>> cfe-commits mailing list
>> cfe-commits at lists.llvm.org <mailto:cfe-commits at lists.llvm.org>
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits <http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits>
> 
> 
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150910/f9d68970/attachment.html>