[cfe-commits] [PATCH] Support for pragma include_alias
Aaron Ballman
aaron at aaronballman.com
Fri Mar 2 14:57:01 PST 2012
On Fri, Mar 2, 2012 at 4:48 PM, Argyrios Kyrtzidis <kyrtzidis at apple.com> wrote:
> On Mar 2, 2012, at 1:33 PM, Aaron Ballman wrote:
>
>> On Fri, Mar 2, 2012 at 12:54 PM, Argyrios Kyrtzidis <kyrtzidis at apple.com> wrote:
>>> On Mar 2, 2012, at 9:51 AM, Aaron Ballman wrote:
>>>
>>>> On Thu, Mar 1, 2012 at 8:07 PM, Argyrios Kyrtzidis <kyrtzidis at apple.com> wrote:
>>>>> On Mar 1, 2012, at 10:43 AM, Aaron Ballman wrote:
>>>>>
>>>>>> On Thu, Mar 1, 2012 at 12:21 AM, Richard Smith <richard at metafoo.co.uk> wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>>>> On Sun, Feb 19, 2012 at 5:31 PM, Aaron Ballman <aaron at aaronballman.com>
>>>>>>>>> wrote:
>>>>>>>>>> One of the MSVC pragmas is the ability to remap include filenames, as
>>>>>>>>>> described in MSDN:
>>>>>>>>>> http://msdn.microsoft.com/en-us/library/wbeh5h91.aspx
>>>>>>>>>>
>>>>>>>>>> The long and short of it is: you can use this pragma to map the source
>>>>>>>>>> filename to a replacement filename (usually for supporting the old 8.3
>>>>>>>>>> filenames on FAT systems, but can be used arbitrarily). For instance:
>>>>>>>>>>
>>>>>>>>>> #pragma include_alias( "FoobarIsReallyLong.h", "FOOBAR~1.H" )
>>>>>>>>>> #include "FoobarIsReallyLong.h" // Actually opens up FOOBAR~1.H
>>>>>>>>>>
>>>>>>>>>> #pragma include_alias( <foo.h>, <bar.h> )
>>>>>>>>>> #include <foo.h> // Actually includes <bar.h>
>>>>>>>>>> #include "foo.h" // Still includes "foo.h"
>>>>>>>>>>
>>>>>>>>>> This patch implements that pragma, and closes Bug 10705.
>>>>>>>
>>>>>>>
>>>>>>> Sorry for the delay!
>>>>>>>
>>>>>>>> Index: include/clang/Basic/DiagnosticLexKinds.td
>>>>>>>> ===================================================================
>>>>>>>> --- include/clang/Basic/DiagnosticLexKinds.td (revision 150897)
>>>>>>>> +++ include/clang/Basic/DiagnosticLexKinds.td (working copy)
>>>>>>>> @@ -292,6 +292,15 @@
>>>>>>>> ExtWarn<"__has_warning expected option name (e.g. \"-Wundef\")">,
>>>>>>>> InGroup<MalformedWarningCheck>;
>>>>>>>>
>>>>>>>> +def warn_pragma_include_alias_mismatch :
>>>>>>>> + ExtWarn<"pragma include_alias requires matching include directives "
>>>>>>>> + "(e.g include_alias(\"foo.h\", \"bar.h\") or "
>>>>>>>> + "include_alias(<foo.h>, <bar.h>))">,
>>>>>>>> + InGroup<UnknownPragmas>;
>>>>>>>
>>>>>>> Can we make this diagnostic clearer? I'd prefer for us to produce different
>>>>>>> text depending on whether the source include was a "include" or a <include>.
>>>>>>> As a first pass:
>>>>>>>
>>>>>>> angle-bracketed include <%0> cannot be aliased to double-quoted include "%1"
>>>>>>> double-quoted include "%0" cannot be aliased to angle-bracketed include <%1>
>>>>>>>
>>>>>>>> +def warn_pragma_include_alias_expected :
>>>>>>>> + ExtWarn<"pragma include_alias expected '%0'">,
>>>>>>>> + InGroup<UnknownPragmas>;
>>>>>>>> +
>>>>>>>> def err__Pragma_malformed : Error<
>>>>>>>> "_Pragma takes a parenthesized string literal">;
>>>>>>>> def err_pragma_comment_malformed : Error<
>>>>>>>> Index: include/clang/Lex/HeaderSearch.h
>>>>>>>> ===================================================================
>>>>>>>> --- include/clang/Lex/HeaderSearch.h (revision 150897)
>>>>>>>> +++ include/clang/Lex/HeaderSearch.h (working copy)
>>>>>>>> @@ -154,6 +154,9 @@
>>>>>>>> llvm::StringMap<const DirectoryEntry *, llvm::BumpPtrAllocator>
>>>>>>>> FrameworkMap;
>>>>>>>>
>>>>>>>> + llvm::StringMap<std::pair<StringRef, bool>, llvm::BumpPtrAllocator>
>>>>>>>> + IncludeAliasMap;
>>>>>>>
>>>>>>> This map doesn't appear to be sufficient: the Microsoft documentation
>>>>>>> indicates that "foo" and <foo> can be aliased to different includes.
>>>>>>>
>>>>>>> Also, the StringRef in the value type here will refer to a string whose
>>>>>>> lifetime has ended.
>>>>>>>
>>>>>>>> +
>>>>>>>> /// HeaderMaps - This is a mapping from FileEntry -> HeaderMap,
>>>>>>>> uniquing
>>>>>>>> /// headermaps. This vector owns the headermap.
>>>>>>>> std::vector<std::pair<const FileEntry*, const HeaderMap*> > HeaderMaps;
>>>>>>>> @@ -217,6 +220,25 @@
>>>>>>>> SystemDirIdx++;
>>>>>>>> }
>>>>>>>>
>>>>>>>> + /// AddHeaderMapping -- Map the source include name to the dest include
>>>>>>>> name
>>>>>>>> + void AddHeaderMapping(const StringRef& Source, const StringRef& Dest,
>>>>>>>
>>>>>>> Generally, please write " &" not "& ". Also, StringRefs are usually passed
>>>>>>> by value.
>>>>>>>
>>>>>>>> + bool IsAngled) {
>>>>>>>> + IncludeAliasMap[Source] = std::make_pair(Dest, IsAngled);
>>>>>>>> + }
>>>>>>>> +
>>>>>>>> + StringRef MapHeader(const StringRef& Source, bool isAngled) {
>>>>>>>> + // Do any filename replacements before anything else
>>>>>>>> + llvm::StringMap<std::pair<StringRef,bool>>::const_iterator iter =
>>>>>>>
>>>>>>> Please capitalize IsAngled and Iter, and write "> >" not ">>". I don't like
>>>>>>> repeating this type here (and especially not missing out the
>>>>>>> llvm::BumpPtrAllocator argument); consider adding a typedef for your
>>>>>>> container type.
>>>>>>>
>>>>>>>> + IncludeAliasMap.find(Source);
>>>>>>>> + if (iter != IncludeAliasMap.end()) {
>>>>>>>> + // If the angling matches, then we've found a replacement
>>>>>>>> + if (iter->second.second == isAngled) {
>>>>>>>> + return iter->second.first;
>>>>>>>> + }
>>>>>>>
>>>>>>> Prevailing llvm/clang style is to omit the braces around a simple
>>>>>>> single-statement body such as this.
>>>>>>>
>>>>>>>> + }
>>>>>>>> + return Source;
>>>>>>>> + }
>>>>>>>> +
>>>>>>>> /// \brief Set the path to the module cache.
>>>>>>>> void setModuleCachePath(StringRef CachePath) {
>>>>>>>> ModuleCachePath = CachePath;
>>>>>>>> Index: include/clang/Lex/Preprocessor.h
>>>>>>>> ===================================================================
>>>>>>>> --- include/clang/Lex/Preprocessor.h (revision 150897)
>>>>>>>> +++ include/clang/Lex/Preprocessor.h (working copy)
>>>>>>>> @@ -1262,6 +1262,7 @@
>>>>>>>> void HandlePragmaMessage(Token &MessageTok);
>>>>>>>> void HandlePragmaPushMacro(Token &Tok);
>>>>>>>> void HandlePragmaPopMacro(Token &Tok);
>>>>>>>> + void HandlePragmaIncludeAlias(Token &Tok);
>>>>>>>> IdentifierInfo *ParsePragmaPushOrPopMacro(Token &Tok);
>>>>>>>>
>>>>>>>> // Return true and store the first token only if any CommentHandler
>>>>>>>> Index: lib/Lex/PPDirectives.cpp
>>>>>>>> ===================================================================
>>>>>>>> --- lib/Lex/PPDirectives.cpp (revision 150897)
>>>>>>>> +++ lib/Lex/PPDirectives.cpp (working copy)
>>>>>>>> @@ -1297,6 +1297,9 @@
>>>>>>>> PragmaARCCFCodeAuditedLoc = SourceLocation();
>>>>>>>> }
>>>>>>>>
>>>>>>>> + // Map the filename
>>>>>>>> + Filename = HeaderInfo.MapHeader(Filename, isAngled);
>>>>>>>> +
>>>>>>>> // Search include directories.
>>>>>>>> const DirectoryLookup *CurDir;
>>>>>>>> SmallString<1024> SearchPath;
>>>>>>>> Index: lib/Lex/Pragma.cpp
>>>>>>>> ===================================================================
>>>>>>>> --- lib/Lex/Pragma.cpp (revision 150897)
>>>>>>>> +++ lib/Lex/Pragma.cpp (working copy)
>>>>>>>> @@ -663,6 +663,101 @@
>>>>>>>> }
>>>>>>>> }
>>>>>>>>
>>>>>>>> +void Preprocessor::HandlePragmaIncludeAlias(Token& Tok) {
>>>>>>>> + // We will either get a quoted filename or a bracketed filename, and
>>>>>>>> we
>>>>>>>> + // have to track which we got. The first filename is the source name,
>>>>>>>> + // and the second name is the mapped filename. If the first is quoted,
>>>>>>>> + // the second must be as well (cannot mix and match quotes and
>>>>>>>> brackets).
>>>>>>>> + SourceLocation Loc = Tok.getLocation();
>>>>>>>> +
>>>>>>>> + // Get the open paren
>>>>>>>> + Lex(Tok);
>>>>>>>> + if (Tok.isNot(tok::l_paren)) {
>>>>>>>> + Diag(Tok, diag::warn_pragma_include_alias_expected) << "(";
>>>>>>>> + return;
>>>>>>>> + }
>>>>>>>> +
>>>>>>>> + // We expect either a quoted string literal, or a bracketed name
>>>>>>>> + Token SourceFilenameTok;
>>>>>>>> + CurPPLexer->LexIncludeFilename(SourceFilenameTok);
>>>>>>>> + if (SourceFilenameTok.is(tok::eod)) {
>>>>>>>> + // The diagnostic has already been handled
>>>>>>>> + return;
>>>>>>>> + }
>>>>>>>> +
>>>>>>>> + StringRef SourceFileName;
>>>>>>>> + SmallString<128> FileNameBuffer;
>>>>>>>> + if (SourceFilenameTok.is(tok::string_literal) ||
>>>>>>>> + SourceFilenameTok.is(tok::angle_string_literal)) {
>>>>>>>> + SourceFileName = getSpelling(SourceFilenameTok, FileNameBuffer);
>>>>>>>> + } else if (SourceFilenameTok.is(tok::less)) {
>>>>>>>> + // This could be a path instead of just a name
>>>>>>>> + FileNameBuffer.push_back('<');
>>>>>>>> + SourceLocation End;
>>>>>>>> + if (ConcatenateIncludeName(FileNameBuffer, End))
>>>>>>>> + return; // Diagnostic already emitted
>>>>>>>> + SourceFileName = FileNameBuffer.str();
>>>>>>>> + } else {
>>>>>>>> + Diag(Tok, diag::warn_pragma_include_alias_expected) << "include
>>>>>>>> filename";
>>>>>>>
>>>>>>> English phrases shouldn't be used as diagnostic arguments, since they won't
>>>>>>> be translatable (if we ever choose to translate the diagnostics). Please use
>>>>>>> a different diagnostic for this; perhaps diag::err_pp_expects_filename?
>>>>>>>
>>>>>>> Also, I don't see a test for this error.
>>>>>>>
>>>>>>>> + return;
>>>>>>>> + }
>>>>>>>> + FileNameBuffer.clear();
>>>>>>>> +
>>>>>>>> + // Now we expect a comma, followed by another include name
>>>>>>>> + Lex(Tok);
>>>>>>>> + if (Tok.isNot(tok::comma)) {
>>>>>>>> + Diag(Tok, diag::warn_pragma_include_alias_expected) << ",";
>>>>>>>> + return;
>>>>>>>> + }
>>>>>>>> +
>>>>>>>> + Token ReplaceFilenameTok;
>>>>>>>> + CurPPLexer->LexIncludeFilename(ReplaceFilenameTok);
>>>>>>>> + if (ReplaceFilenameTok.is(tok::eod)) {
>>>>>>>> + // The diagnostic has already been handled
>>>>>>>> + return;
>>>>>>>> + }
>>>>>>>> +
>>>>>>>> + StringRef ReplaceFileName;
>>>>>>>> + if (ReplaceFilenameTok.is(tok::string_literal) ||
>>>>>>>> + ReplaceFilenameTok.is(tok::angle_string_literal)) {
>>>>>>>> + ReplaceFileName = getSpelling(ReplaceFilenameTok, FileNameBuffer);
>>>>>>>> + } else if (ReplaceFilenameTok.is(tok::less)) {
>>>>>>>> + // This could be a path instead of just a name
>>>>>>>> + FileNameBuffer.push_back('<');
>>>>>>>> + SourceLocation End;
>>>>>>>> + if (ConcatenateIncludeName(FileNameBuffer, End))
>>>>>>>> + return; // Diagnostic already emitted
>>>>>>>> + ReplaceFileName = FileNameBuffer.str();
>>>>>>>> + } else {
>>>>>>>> + Diag(Tok, diag::warn_pragma_include_alias_expected) << "include
>>>>>>>> filename";
>>>>>>>> + return;
>>>>>>>> + }
>>>>>>>
>>>>>>> Could this repeated code be factored out? (I imagine we have largely the
>>>>>>> same pattern in some other places too.)
>>>>>>>
>>>>>>>> +
>>>>>>>> + // Finally, we expect the closing paren
>>>>>>>> + Lex(Tok);
>>>>>>>> + if (Tok.isNot(tok::r_paren)) {
>>>>>>>> + Diag(Tok, diag::warn_pragma_include_alias_expected) << ")";
>>>>>>>> + return;
>>>>>>>> + }
>>>>>>>> +
>>>>>>>> + // Now that we have the source and target filenames, we need to make
>>>>>>>> sure
>>>>>>>> + // they're both of the same type (angled vs non-angled)
>>>>>>>> + bool SourceIsAngled =
>>>>>>>> + GetIncludeFilenameSpelling(SourceFilenameTok.getLocation(),
>>>>>>>> + SourceFileName);
>>>>>>>> + bool ReplaceIsAngled =
>>>>>>>> + GetIncludeFilenameSpelling(ReplaceFilenameTok.getLocation(),
>>>>>>>> + ReplaceFileName);
>>>>>>>> + if (SourceIsAngled != ReplaceIsAngled) {
>>>>>>>> + Diag(Loc, diag::warn_pragma_include_alias_mismatch);
>>>>>>>> + return;
>>>>>>>> + }
>>>>>>>> +
>>>>>>>> + // Now we can let the include handler know about this mapping
>>>>>>>> + getHeaderSearchInfo().AddHeaderMapping(SourceFileName,
>>>>>>>> ReplaceFileName,
>>>>>>>
>>>>>>> ReplaceFileName here refers to a string which expires at the end of this
>>>>>>> function, and it's being saved away by AddHeaderMapping for later use.
>>>>>>>
>>>>>>>> + SourceIsAngled);
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> /// AddPragmaHandler - Add the specified pragma handler to the
>>>>>>>> preprocessor.
>>>>>>>> /// If 'Namespace' is non-null, then it is a token required to exist on
>>>>>>>> the
>>>>>>>> /// pragma line before the pragma string starts, e.g. "STDC" or "GCC".
>>>>>>>> @@ -943,6 +1038,15 @@
>>>>>>>> }
>>>>>>>> };
>>>>>>>>
>>>>>>>> +/// PragmaIncludeAliasHandler - "#pragma include_alias("...")".
>>>>>>>> +struct PragmaIncludeAliasHandler : public PragmaHandler {
>>>>>>>> + PragmaIncludeAliasHandler() : PragmaHandler("include_alias") {}
>>>>>>>> + virtual void HandlePragma(Preprocessor &PP, PragmaIntroducerKind
>>>>>>>> Introducer,
>>>>>>>> + Token &IncludeAliasTok) {
>>>>>>>> + PP.HandlePragmaIncludeAlias(IncludeAliasTok);
>>>>>>>> + }
>>>>>>>> +};
>>>>>>>> +
>>>>>>>> /// PragmaMessageHandler - "#pragma message("...")".
>>>>>>>> struct PragmaMessageHandler : public PragmaHandler {
>>>>>>>> PragmaMessageHandler() : PragmaHandler("message") {}
>>>>>>>> @@ -1095,5 +1199,6 @@
>>>>>>>> // MS extensions.
>>>>>>>> if (Features.MicrosoftExt) {
>>>>>>>> AddPragmaHandler(new PragmaCommentHandler());
>>>>>>>> + AddPragmaHandler(new PragmaIncludeAliasHandler());
>>>>>>>> }
>>>>>>>> }
>>>>>>>> Index: test/Preprocessor/pragma_microsoft.c
>>>>>>>> ===================================================================
>>>>>>>> --- test/Preprocessor/pragma_microsoft.c (revision 150897)
>>>>>>>> +++ test/Preprocessor/pragma_microsoft.c (working copy)
>>>>>>>> @@ -38,3 +38,20 @@
>>>>>>>> // this warning should go away.
>>>>>>>> MACRO_WITH__PRAGMA // expected-warning {{expression result unused}}
>>>>>>>> }
>>>>>>>> +
>>>>>>>> +
>>>>>>>> +// This should include macro_arg_directive even though the include
>>>>>>>> +// is looking for test.h This allows us to assign to "n"
>>>>>>>> +#pragma include_alias("test.h", "macro_arg_directive.h" )
>>>>>>>> +#include "test.h"
>>>>>>>> +void test( void ) {
>>>>>>>> + n = 12;
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +#pragma include_alias("foo.h", <bar.h>) // expected-warning {{pragma
>>>>>>>> include_alias requires matching include directives (e.g
>>>>>>>> include_alias("foo.h", "bar.h") or include_alias(<foo.h>, <bar.h>))}}
>>>>>>>> +#pragma include_alias("test.h") // expected-warning {{pragma
>>>>>>>> include_alias expected ','}}
>>>>>>>> +
>>>>>>>> +// Make sure that the names match exactly for a replacement, including
>>>>>>>> path information. If
>>>>>>>> +// this were to fail, we would get a file not found error
>>>>>>>> +#pragma include_alias(".\pp-record.h", "does_not_exist.h")
>>>>>>>> +#include "pp-record.h"
>>>>>>>
>>>>>>
>>>>>> Hi Richard -- I've addressed almost all of the points in your review,
>>>>>> and have attached a patch for you to check over again. Thank you for
>>>>>> all the helpful feedback!
>>>>>>
>>>>>> The only thing I didn't address was:
>>>>>>
>>>>>>> Could this repeated code be factored out? (I imagine we have largely the
>>>>>>> same pattern in some other places too.)
>>>>>>
>>>>>> I'm guessing the code could be refactored out, but I'd rather that be
>>>>>> a separate patch since it touches on more than just include_alias.
>>>>>>
>>>>>> Thanks!
>>>>>
>>>>> A few additional comments, I suggest optimizing for the common path, that there is no include_alias in the source, so:
>>>>>
>>>>> + typedef llvm::StringMap<std::string, llvm::BumpPtrAllocator>
>>>>> + IncludeAliasMap;
>>>>> + IncludeAliasMap IncludeAliases;
>>>>>
>>>>> Have this be a pointer and create the object lazily on demand:
>>>>> OwningPtr<IncludeAliasMap> IncludeAliases;
>>>>>
>>>>> + // Map the filename with the brackets still attached. If the name doesn't
>>>>> + // map to anything, fall back on the filename we've already gotten the
>>>>> + // spelling for.
>>>>> + std::string NewName = HeaderInfo.MapHeader(OriginalFilename);
>>>>> + if (!NewName.empty())
>>>>> + Filename = NewName;
>>>>> +
>>>>>
>>>>> here you can check if the object was created or not before going ahead to call HeaderInfo.MapHeader(OriginalFilename).
>>>>>
>>>>> + void AddHeaderMapping(StringRef Source, StringRef Dest) {
>>>>> and
>>>>> + std::string MapHeader(StringRef Source) {
>>>>>
>>>>> Rename to "AddIncludeAlias" and "MapHeaderToIncludeAlias" to make clear what these are for ?
>>>>
>>>> Thanks for the reviews -- I've attached a patch with your suggestions
>>>> incorporated. I've cleaned up the items Richard pointed out, and made
>>>> the alias mapping lazy-loaded as Argyrios suggested.
>>>
>>> Thanks Aaron. Another thing I'd like to bring to your attention:
>>>
>>> Since all the include_alias related methods are only used by the preprocessor, how about moving the methods and IncludeAliases object to the Preprocessor and make them all private ?
>>
>> I've been thinking about this, and it doesn't sit quite right with me.
>> Since this is so heavily related to header files, I think it's a bit
>> cleaner to pack it in with the HeaderSearch functionality. Keeps all
>> the logic in one place.
>>
>> Do you have strong feelings against that?
>
> No, committing is fine by me.
Thank you both for your excellent reviews. :-) Committed in 151949.
~Aaron
More information about the cfe-commits
mailing list