On Fri, Mar 2, 2012 at 4:48 PM, Argyrios Kyrtzidis <[email protected]> wrote: > On Mar 2, 2012, at 1:33 PM, Aaron Ballman wrote: > >> On Fri, Mar 2, 2012 at 12:54 PM, Argyrios Kyrtzidis <[email protected]> >> wrote: >>> 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'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 _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
