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? ~Aaron _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
