[cfe-commits] [PATCH] Support for pragma include_alias

[Aaron Ballman]

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?

~Aaron