[cfe-commits] r134587 - in /cfe/trunk: include/clang/Basic/SourceManager.h include/clang/Lex/MacroInfo.h include/clang/Lex/TokenLexer.h lib/ARCMigrate/TransformActions.cpp lib/ARCMigrate/Transforms.cpp lib/Basic/SourceManager.cpp lib/Lex/Lexer.cp
Argyrios Kyrtzidis
kyrtzidis at apple.com
Thu Jul 7 14:19:03 PDT 2011
On Jul 7, 2011, at 1:53 PM, Argyrios Kyrtzidis wrote:
> On Jul 7, 2011, at 11:43 AM, Chandler Carruth wrote:
>
>> Redone patch for PR9279 attached.
>
> Love the new diagnostics! Looks good, please commit.
Ah, one comment:
--- a/lib/Lex/TokenLexer.cpp
+++ b/lib/Lex/TokenLexer.cpp
@@ -226,9 +226,9 @@ void TokenLexer::ExpandFunctionArguments() {
"Expected arg identifier to come from definition");
for (unsigned i = FirstResult, e = ResultToks.size(); i != e; ++i) {
Token &Tok = ResultToks[i];
- Tok.setLocation(SM.createInstantiationLoc(Tok.getLocation(),
- curInst, curInst,
- Tok.getLength()));
+ Tok.setLocation(SM.createMacroArgInstantiationLoc(Tok.getLocation(),
+ curInst,
+ Tok.getLength()));
}
}
Also change the following SM.createInstantiationLoc below this, to handle arguments for the "## __VA_ARGS__" code path, e.g, try this test case:
#define macro_args1(x) x
#define macro_args2(x) macro_args1(x)
#define macro_args3(x, ...) macro_args2(({x ## __VA_ARGS__}))
void test() {
macro_args3(1,2,3,4);
}
-Argiris
>
>>
>> On Thu, Jul 7, 2011 at 10:22 AM, Argyrios Kyrtzidis <kyrtzidis at apple.com> wrote:
>> Hi Chandler,
>>
>> My patch is not as disruptive as you think, I think you only need to apply the same patch that reworks how diagnostics are emitted. Note that I didn't touch that part which I assume is the important part of PR9279.
>> The actual driving force for pointing to macro arguments was not to improve diagnostics but to have SourceManager::isBeforeInTranslationUnit keep working. My assumption was that your reworking of the diagnostic printer would come on top.
>>
>> -Argiris
>>
>> On Jul 7, 2011, at 10:05 AM, Chandler Carruth wrote:
>>
>>> I'm really frustrated at this development. I'm now going to have to completely re-work my patch for PR9279. This commit actually applies several pieces of an early version of my patch for that bug, without handling the problems that arose from those approaches, or addressing the test cases there.
>>>
>>> Why can't we fix the underlying problem first and then do this sort of optimization work? I really like the memory savings and other changes, but we have active bugs with the diagnostics that are still not addressed. Every time we change things underneath, it makes it harder to forward-port the fix I posted.
>>>
>>> It would have also been really useful to provide feedback on those patches. I would have been happy to work on the memory restructuring you've done here on top of them and we could have been faster and more correct a long time ago...
>>>
>>> On to the specific problems:
>>>
>>> On Wed, Jul 6, 2011 at 8:40 PM, Argyrios Kyrtzidis <akyrtzi at gmail.com> wrote:
>>> t.c:5:9: error: invalid operands to binary expression ('struct S' and 'int')
>>> int y = M(/);
>>> ^~~~
>>> t.c:3:20: note: instantiated from:
>>> \#define M(op) (foo op 3);
>>> ~~~ ^ ~
>>> t.c:5:11: note: instantiated from:
>>> int y = M(/);
>>> ^
>>>
>>> I think re-showing line 5 here is really unfortunate. We already have a problem with too many lines being displayed for diagnostics. Why can't we start off the trace with the macro argument, and trace the macro argument through each level of instantiation? You're already recording most of the information necessary to do this (as you have several of the key elements of my patch for PR9279 in your patch). For example, for (what seems close to) your test case with my patch applied from PR9279 Clang prints the following:
>>>
>>> t.c:4:13: error: invalid operands to binary expression ('struct S' and 'int')
>>> int y = M(/);
>>> ~~^~
>>> t.c:3:20: note: instantiated from:
>>> #define M(op) (foo op 3)
>>> ~~~ ^ ~
>>> 1 error generated.
>>>
>>> I think that's a strictly better diagnostic experience. Several of the test cases I included in that PR (and split into other PRs) highlight the difference. I think the best example is:
>>>
>>> % cat t5.cc
>>> #define M0 4
>>> #define M1(x, y, z) y
>>> #define M2(x, y, z) M1(x, y, z)
>>> #define M3(x, y, z) M2(x, y, z)
>>> M3(
>>> 1,
>>> M0,
>>> 3);
>>>
>>> Which with your patch gives:
>>> % ./bin/clang -fsyntax-only t5.cc
>>> t5.cc:5:1: error: expected unqualified-id
>>> M3(
>>> ^
>>> t5.cc:4:21: note: instantiated from:
>>> #define M3(x, y, z) M2(x, y, z)
>>> ^
>>> t5.cc:3:21: note: instantiated from:
>>> #define M2(x, y, z) M1(x, y, z)
>>> ^
>>> t5.cc:2:21: note: instantiated from:
>>> #define M1(x, y, z) y
>>> ^
>>> t5.cc:5:1: note: instantiated from:
>>> #define M0 4
>>> ^
>>> 1 error generated.
>>>
>>> We don't see where the user wrote "M0" here. We also don't see where 'y' is used within each macro. My patch produces:
>>> % ./bin/clang -fsyntax-only t5.cc
>>> t5.cc:7:3: error: expected unqualified-id
>>> M0,
>>> ^
>>> t5.cc:1:12: note: instantiated from:
>>> #define M0 4
>>> ^
>>> t5.cc:4:27: note: instantiated from:
>>> #define M3(x, y, z) M2(x, y, z)
>>> ^
>>> t5.cc:3:27: note: instantiated from:
>>> #define M2(x, y, z) M1(x, y, z)
>>> ^
>>> t5.cc:2:21: note: instantiated from:
>>> #define M1(x, y, z) y
>>> ^
>>> 1 error generated.
>>
>>
>> <PR9279.patch>_______________________________________________
>> cfe-commits mailing list
>> cfe-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20110707/88c8f328/attachment.html>
More information about the cfe-commits
mailing list