[cfe-commits] [PATCH] Support for pragma include_alias
Richard Smith
richard at metafoo.co.uk
Fri Mar 2 13:55:24 PST 2012
On Fri, Mar 2, 2012 at 9:51 AM, Aaron Ballman <aaron at aaronballman.com>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.
>
Minor whitespacey things:
+ // Do any filename replacements before anything else
+ IncludeAliasMap::const_iterator Iter =
+ IncludeAliases->find(Source);
This will fit on a single line.
+ if (Iter != IncludeAliases->end())
+ return Iter->second;
This is indented too far.
Other than that, you've addressed all of my concerns, but please don't
commit until Argyrios is happy too.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120302/9eb89daa/attachment.html>
More information about the cfe-commits
mailing list