[cfe-commits] r132247 - in /cfe/trunk: include/clang/Basic/SourceManager.h lib/Basic/SourceManager.cpp

[Argyrios Kyrtzidis]

On May 28, 2011, at 2:52 PM, Chandler Carruth wrote:

> On Sat, May 28, 2011 at 2:02 PM, Argyrios Kyrtzidis <akyrtzi at gmail.com> wrote:
>> On May 28, 2011, at 12:50 AM, Chandler Carruth wrote:
>> 
>> On Sat, May 28, 2011 at 12:45 AM, Abramo Bagnara
>> <abramo.bagnara at gmail.com> wrote:
>> 
>> This is *very* useful!
>> 
>> Using similar technique do you think it is feasible to do an helper that
>> 
>> permit to know if a macro token is at start/end of a macro argument?
>> 
>> Before you start touching macro argument source locations, please look
>> at P9279 and my last patch. I'm working on the final benchmarks to
>> validate that this doesn't cause major performance problems for
>> typical code. Hopefully I'll get that checked in this weekend, but I
>> thought that the last time I rotated around to work on that....
>> 
>> % ./new/clang -fsyntax-only t6.cc
>> t6.cc:5:1: error: expected unqualified-id
>> M3(1, M0(2), 3);
>>         ^
>> t6.cc:1:15: note: instantiated from:
>> #define M0(x) x
>>              ^
>> t6.cc:4:27: note: instantiated from:
>> #define M3(x, y, z) M2(x, y, z)
>>                          ^
>> t6.cc:3:27: note: instantiated from:
>> #define M2(x, y, z) M1(x, y, z)
>>                          ^
>> t6.cc:2:21: note: instantiated from:
>> #define M1(x, y, z) y
>>                    ^
>> 1 error generated.
>> 
>> That is some serious diagnostic quality improvement, kudos Chandler!
>> That said I think we can get the same effect without storing extra source
>> locations, I'll look into it more..
> 
> I'm not sure how, I tried 3 different approaches before settling on
> that one. And I also don't think storing extra locations is a problem.
> The only way I could measure a performance drop was to take an extreme
> edge case for macro arguments (GCC's source), and to run it is -Eonly
> mode. The moment I do something else, and the performance hit drops
> beneath the noise floor. Especially on typical code.
> 
> I should have a benchmark today that clearly demonstrates this, and
> then I'll check in the patch.
> 
> One thing to remember, while we might be able to conjure up the
> diagnostic improvements, having the full information for each
> direction of the macro expansion really helps the precision of
> refactoring and rewriting tools in the presence of macro arguments. We
> have cases where we need to re-write the contents of macro bodies,
> etc., and this level of information really helps that.

I completely agree and the extra info about argument expansion is a great improvement but it still doesn't provide *all* the info a client may need to fully explore a macro expansion.

I think what a client ultimately needs (and hopefully Abramo may offer further insights) is 2 things, a list of all the locations of tokens of the macro expansion and the source location of the macro definition that was responsible for the expansion.
All info that a client may need can be derived/reconstructed by these 2 pieces of info and we can offer helper routines for the more commonly useful info (e.g. for the position of the macro argument, or the source range of the input to the macro argument).

We can already get a list of tokens, we just don't expose it to clients; we can provide a list of all the source locations of the macro tokens and the client can derive a list of the spelling locations/tokens.
We are unable to provide the location of the macro definition responsible for the expansion because we don't keep it anywhere. Maybe add a SourceLocation in InstantiationInfo ? It won't increase the size of SLocEntry in x86_64 due to alignment of FileInfo. Or add a source location the way you did for macro argument expansion.

Comments ?