[cfe-commits] r164874 - in /cfe/trunk: include/clang/Lex/PPCallbacks.h include/clang/Lex/PreprocessingRecord.h lib/Frontend/DependencyFile.cpp lib/Frontend/DependencyGraph.cpp lib/Lex/PPDirectives.cpp lib/Lex/PreprocessingRecord.cpp lib/Lex/Prepr

[Kim Gräsman]

Hi Richard, all,

On Mon, Oct 1, 2012 at 8:31 PM, Richard Smith <richard at metafoo.co.uk> wrote:
>
> Yes, I think we should maintain SourceLocation fidelity wherever possible.
> Clients can convert to the expansion location themselves if they need to.
>
> It looks like the CharEnd calculation for the string_literal and
> angle_string_literal cases is wrong:
>
>     Filename = getSpelling(FilenameTok, FilenameBuffer);
>     End = FilenameTok.getLocation();
>     CharEnd = End.getLocWithOffset(Filename.size());
>
> You can't use getLocWithOffset to find the end of a token this way (it won't
> work for trigraphs, at least). This should be using getLocForEndOfToken,
> except that that apparently doesn't work inside a macro expansion. (That
> looks like the same problem which is breaking Kim's angled-include-macro
> case).

Thanks for the hints!

There seems to be two problems:

1) In the case of literal tokens, CharEnd was previously based on
Filename.size(). This fails to take trigraphs into account. It looks
like it works better with FilenameTok.getLength():

   case tok::angle_string_literal:
   case tok::string_literal:
     Filename = getSpelling(FilenameTok, FilenameBuffer);
     End = FilenameTok.getLocation();
-    CharEnd = End.getLocWithOffset(Filename.size());
+    CharEnd = End.getLocWithOffset(FilenameTok.getLength());
     break;


2) In the case of the less token (triggered by macro expansions that
result in angled includes), CharEnd was based on End, which had been
advanced by ConcatenateIncludeName. End already points to the closing
'>', so I changed it to:

   case tok::less:
     // This could be a <foo/bar.h> file coming from a macro expansion.  In this
     // case, glue the tokens together into FilenameBuffer and interpret those.
     FilenameBuffer.push_back('<');
     if (ConcatenateIncludeName(FilenameBuffer, End))
       return;   // Found <eod> but no ">"?  Diagnostic already emitted.
     Filename = FilenameBuffer.str();
-    CharEnd = getLocForEndOfToken(End);
+
+    CharEnd = End.getLocWithOffset(1);
     break;

I've tested these by running my callback impl over a simple .cc file,
and I get the results I expect, i.e. the FilenameRange contains the
macro "body" (this is the spelling location, right?);

--
#define MACRO_TRIGRAPH "trigraph??-empty.h"
#define MACRO_ANGLED <vadefs.h>
#define MACRO_QUOTED "empty.h"
#define MACRO_STRINGIZED(x) #x

#include MACRO_QUOTED
#include MACRO_ANGLED
#include MACRO_TRIGRAPH
#include MACRO_STRINGIZED(empty.h)

#include <vadefs.h>
#include "empty.h"
--

(<vadefs.h> is an MSVC header that has no further includes, so as not
to muddy the results)

Is there test coverage for this stuff somewhere? I'm not familiar with
Clang's testing tools yet, but I would make an effort to get this
under test if I knew where to start...

Thanks!

- Kim