On Mar 2, 2012, at 9:51 AM, Aaron Ballman wrote: > On Thu, Mar 1, 2012 at 8:07 PM, Argyrios Kyrtzidis <[email protected]> > wrote: >> On Mar 1, 2012, at 10:43 AM, Aaron Ballman wrote: >> >>> On Thu, Mar 1, 2012 at 12:21 AM, Richard Smith <[email protected]> >>> wrote: >>>> Hi, >>>> >>>>>> On Sun, Feb 19, 2012 at 5:31 PM, Aaron Ballman <[email protected]> >>>>>> 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'd prefer if such esoteric extensions were isolated and hidden away as much as possible, like in a dark dungeon :-) > > Thanks! > > ~Aaron > <include_alias.patch> _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
