[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 chandlerc at google.com
Thu Jul 7 10:05:57 PDT 2011


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/57dd1f39/attachment.html>


More information about the cfe-commits mailing list