[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

[Chandler Carruth]

Redone patch for PR9279 attached.

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.
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20110707/548b3c1b/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: PR9279.patch
Type: application/octet-stream
Size: 20421 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20110707/548b3c1b/attachment.obj>