On Fri, Mar 2, 2012 at 9:51 AM, Aaron Ballman <[email protected]>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. > 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.
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
