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