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