<div class="gmail_quote">On Fri, Mar 2, 2012 at 9:51 AM, Aaron Ballman <span dir="ltr"><<a href="mailto:aaron@aaronballman.com">aaron@aaronballman.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="HOEnZb"><div class="h5">On Thu, Mar 1, 2012 at 8:07 PM, Argyrios Kyrtzidis <<a href="mailto:kyrtzidis@apple.com">kyrtzidis@apple.com</a>> wrote:<br>
> On Mar 1, 2012, at 10:43 AM, Aaron Ballman wrote:<br>
><br>
>> On Thu, Mar 1, 2012 at 12:21 AM, Richard Smith <<a href="mailto:richard@metafoo.co.uk">richard@metafoo.co.uk</a>> wrote:<br>
>>> Hi,<br>
>>><br>
>>>>> On Sun, Feb 19, 2012 at 5:31 PM, Aaron Ballman <<a href="mailto:aaron@aaronballman.com">aaron@aaronballman.com</a>><br>
>>>>> wrote:<br>
>>>>>> One of the MSVC pragmas is the ability to remap include filenames, as<br>
>>>>>> described in MSDN:<br>
>>>>>> <a href="http://msdn.microsoft.com/en-us/library/wbeh5h91.aspx" target="_blank">http://msdn.microsoft.com/en-us/library/wbeh5h91.aspx</a><br>
>>>>>><br>
>>>>>> The long and short of it is: you can use this pragma to map the source<br>
>>>>>> filename to a replacement filename (usually for supporting the old 8.3<br>
>>>>>> filenames on FAT systems, but can be used arbitrarily). For instance:<br>
>>>>>><br>
>>>>>> #pragma include_alias( "FoobarIsReallyLong.h", "FOOBAR~1.H" )<br>
>>>>>> #include "FoobarIsReallyLong.h" // Actually opens up FOOBAR~1.H<br>
>>>>>><br>
>>>>>> #pragma include_alias( <foo.h>, <bar.h> )<br>
>>>>>> #include <foo.h> // Actually includes <bar.h><br>
>>>>>> #include "foo.h" // Still includes "foo.h"<br>
>>>>>><br>
>>>>>> This patch implements that pragma, and closes Bug 10705.<br>
>>><br>
>>><br>
>>> Sorry for the delay!<br>
>>><br>
>>>> Index: include/clang/Basic/DiagnosticLexKinds.td<br>
>>>> ===================================================================<br>
>>>> --- include/clang/Basic/DiagnosticLexKinds.td (revision 150897)<br>
>>>> +++ include/clang/Basic/DiagnosticLexKinds.td (working copy)<br>
>>>> @@ -292,6 +292,15 @@<br>
>>>> ExtWarn<"__has_warning expected option name (e.g. \"-Wundef\")">,<br>
>>>> InGroup<MalformedWarningCheck>;<br>
>>>><br>
>>>> +def warn_pragma_include_alias_mismatch :<br>
>>>> + ExtWarn<"pragma include_alias requires matching include directives "<br>
>>>> + "(e.g include_alias(\"foo.h\", \"bar.h\") or "<br>
>>>> + "include_alias(<foo.h>, <bar.h>))">,<br>
>>>> + InGroup<UnknownPragmas>;<br>
>>><br>
>>> Can we make this diagnostic clearer? I'd prefer for us to produce different<br>
>>> text depending on whether the source include was a "include" or a <include>.<br>
>>> As a first pass:<br>
>>><br>
>>> angle-bracketed include <%0> cannot be aliased to double-quoted include "%1"<br>
>>> double-quoted include "%0" cannot be aliased to angle-bracketed include <%1><br>
>>><br>
>>>> +def warn_pragma_include_alias_expected :<br>
>>>> + ExtWarn<"pragma include_alias expected '%0'">,<br>
>>>> + InGroup<UnknownPragmas>;<br>
>>>> +<br>
>>>> def err__Pragma_malformed : Error<<br>
>>>> "_Pragma takes a parenthesized string literal">;<br>
>>>> def err_pragma_comment_malformed : Error<<br>
>>>> Index: include/clang/Lex/HeaderSearch.h<br>
>>>> ===================================================================<br>
>>>> --- include/clang/Lex/HeaderSearch.h (revision 150897)<br>
>>>> +++ include/clang/Lex/HeaderSearch.h (working copy)<br>
>>>> @@ -154,6 +154,9 @@<br>
>>>> llvm::StringMap<const DirectoryEntry *, llvm::BumpPtrAllocator><br>
>>>> FrameworkMap;<br>
>>>><br>
>>>> + llvm::StringMap<std::pair<StringRef, bool>, llvm::BumpPtrAllocator><br>
>>>> + IncludeAliasMap;<br>
>>><br>
>>> This map doesn't appear to be sufficient: the Microsoft documentation<br>
>>> indicates that "foo" and <foo> can be aliased to different includes.<br>
>>><br>
>>> Also, the StringRef in the value type here will refer to a string whose<br>
>>> lifetime has ended.<br>
>>><br>
>>>> +<br>
>>>> /// HeaderMaps - This is a mapping from FileEntry -> HeaderMap,<br>
>>>> uniquing<br>
>>>> /// headermaps. This vector owns the headermap.<br>
>>>> std::vector<std::pair<const FileEntry*, const HeaderMap*> > HeaderMaps;<br>
>>>> @@ -217,6 +220,25 @@<br>
>>>> SystemDirIdx++;<br>
>>>> }<br>
>>>><br>
>>>> + /// AddHeaderMapping -- Map the source include name to the dest include<br>
>>>> name<br>
>>>> + void AddHeaderMapping(const StringRef& Source, const StringRef& Dest,<br>
>>><br>
>>> Generally, please write " &" not "& ". Also, StringRefs are usually passed<br>
>>> by value.<br>
>>><br>
>>>> + bool IsAngled) {<br>
>>>> + IncludeAliasMap[Source] = std::make_pair(Dest, IsAngled);<br>
>>>> + }<br>
>>>> +<br>
>>>> + StringRef MapHeader(const StringRef& Source, bool isAngled) {<br>
>>>> + // Do any filename replacements before anything else<br>
>>>> + llvm::StringMap<std::pair<StringRef,bool>>::const_iterator iter =<br>
>>><br>
>>> Please capitalize IsAngled and Iter, and write "> >" not ">>". I don't like<br>
>>> repeating this type here (and especially not missing out the<br>
>>> llvm::BumpPtrAllocator argument); consider adding a typedef for your<br>
>>> container type.<br>
>>><br>
>>>> + IncludeAliasMap.find(Source);<br>
>>>> + if (iter != IncludeAliasMap.end()) {<br>
>>>> + // If the angling matches, then we've found a replacement<br>
>>>> + if (iter->second.second == isAngled) {<br>
>>>> + return iter->second.first;<br>
>>>> + }<br>
>>><br>
>>> Prevailing llvm/clang style is to omit the braces around a simple<br>
>>> single-statement body such as this.<br>
>>><br>
>>>> + }<br>
>>>> + return Source;<br>
>>>> + }<br>
>>>> +<br>
>>>> /// \brief Set the path to the module cache.<br>
>>>> void setModuleCachePath(StringRef CachePath) {<br>
>>>> ModuleCachePath = CachePath;<br>
>>>> Index: include/clang/Lex/Preprocessor.h<br>
>>>> ===================================================================<br>
>>>> --- include/clang/Lex/Preprocessor.h (revision 150897)<br>
>>>> +++ include/clang/Lex/Preprocessor.h (working copy)<br>
>>>> @@ -1262,6 +1262,7 @@<br>
>>>> void HandlePragmaMessage(Token &MessageTok);<br>
>>>> void HandlePragmaPushMacro(Token &Tok);<br>
>>>> void HandlePragmaPopMacro(Token &Tok);<br>
>>>> + void HandlePragmaIncludeAlias(Token &Tok);<br>
>>>> IdentifierInfo *ParsePragmaPushOrPopMacro(Token &Tok);<br>
>>>><br>
>>>> // Return true and store the first token only if any CommentHandler<br>
>>>> Index: lib/Lex/PPDirectives.cpp<br>
>>>> ===================================================================<br>
>>>> --- lib/Lex/PPDirectives.cpp (revision 150897)<br>
>>>> +++ lib/Lex/PPDirectives.cpp (working copy)<br>
>>>> @@ -1297,6 +1297,9 @@<br>
>>>> PragmaARCCFCodeAuditedLoc = SourceLocation();<br>
>>>> }<br>
>>>><br>
>>>> + // Map the filename<br>
>>>> + Filename = HeaderInfo.MapHeader(Filename, isAngled);<br>
>>>> +<br>
>>>> // Search include directories.<br>
>>>> const DirectoryLookup *CurDir;<br>
>>>> SmallString<1024> SearchPath;<br>
>>>> Index: lib/Lex/Pragma.cpp<br>
>>>> ===================================================================<br>
>>>> --- lib/Lex/Pragma.cpp (revision 150897)<br>
>>>> +++ lib/Lex/Pragma.cpp (working copy)<br>
>>>> @@ -663,6 +663,101 @@<br>
>>>> }<br>
>>>> }<br>
>>>><br>
>>>> +void Preprocessor::HandlePragmaIncludeAlias(Token& Tok) {<br>
>>>> + // We will either get a quoted filename or a bracketed filename, and<br>
>>>> we<br>
>>>> + // have to track which we got. The first filename is the source name,<br>
>>>> + // and the second name is the mapped filename. If the first is quoted,<br>
>>>> + // the second must be as well (cannot mix and match quotes and<br>
>>>> brackets).<br>
>>>> + SourceLocation Loc = Tok.getLocation();<br>
>>>> +<br>
>>>> + // Get the open paren<br>
>>>> + Lex(Tok);<br>
>>>> + if (Tok.isNot(tok::l_paren)) {<br>
>>>> + Diag(Tok, diag::warn_pragma_include_alias_expected) << "(";<br>
>>>> + return;<br>
>>>> + }<br>
>>>> +<br>
>>>> + // We expect either a quoted string literal, or a bracketed name<br>
>>>> + Token SourceFilenameTok;<br>
>>>> + CurPPLexer->LexIncludeFilename(SourceFilenameTok);<br>
>>>> + if (SourceFilenameTok.is(tok::eod)) {<br>
>>>> + // The diagnostic has already been handled<br>
>>>> + return;<br>
>>>> + }<br>
>>>> +<br>
>>>> + StringRef SourceFileName;<br>
>>>> + SmallString<128> FileNameBuffer;<br>
>>>> + if (SourceFilenameTok.is(tok::string_literal) ||<br>
>>>> + SourceFilenameTok.is(tok::angle_string_literal)) {<br>
>>>> + SourceFileName = getSpelling(SourceFilenameTok, FileNameBuffer);<br>
>>>> + } else if (SourceFilenameTok.is(tok::less)) {<br>
>>>> + // This could be a path instead of just a name<br>
>>>> + FileNameBuffer.push_back('<');<br>
>>>> + SourceLocation End;<br>
>>>> + if (ConcatenateIncludeName(FileNameBuffer, End))<br>
>>>> + return; // Diagnostic already emitted<br>
>>>> + SourceFileName = FileNameBuffer.str();<br>
>>>> + } else {<br>
>>>> + Diag(Tok, diag::warn_pragma_include_alias_expected) << "include<br>
>>>> filename";<br>
>>><br>
>>> English phrases shouldn't be used as diagnostic arguments, since they won't<br>
>>> be translatable (if we ever choose to translate the diagnostics). Please use<br>
>>> a different diagnostic for this; perhaps diag::err_pp_expects_filename?<br>
>>><br>
>>> Also, I don't see a test for this error.<br>
>>><br>
>>>> + return;<br>
>>>> + }<br>
>>>> + FileNameBuffer.clear();<br>
>>>> +<br>
>>>> + // Now we expect a comma, followed by another include name<br>
>>>> + Lex(Tok);<br>
>>>> + if (Tok.isNot(tok::comma)) {<br>
>>>> + Diag(Tok, diag::warn_pragma_include_alias_expected) << ",";<br>
>>>> + return;<br>
>>>> + }<br>
>>>> +<br>
>>>> + Token ReplaceFilenameTok;<br>
>>>> + CurPPLexer->LexIncludeFilename(ReplaceFilenameTok);<br>
>>>> + if (ReplaceFilenameTok.is(tok::eod)) {<br>
>>>> + // The diagnostic has already been handled<br>
>>>> + return;<br>
>>>> + }<br>
>>>> +<br>
>>>> + StringRef ReplaceFileName;<br>
>>>> + if (ReplaceFilenameTok.is(tok::string_literal) ||<br>
>>>> + ReplaceFilenameTok.is(tok::angle_string_literal)) {<br>
>>>> + ReplaceFileName = getSpelling(ReplaceFilenameTok, FileNameBuffer);<br>
>>>> + } else if (ReplaceFilenameTok.is(tok::less)) {<br>
>>>> + // This could be a path instead of just a name<br>
>>>> + FileNameBuffer.push_back('<');<br>
>>>> + SourceLocation End;<br>
>>>> + if (ConcatenateIncludeName(FileNameBuffer, End))<br>
>>>> + return; // Diagnostic already emitted<br>
>>>> + ReplaceFileName = FileNameBuffer.str();<br>
>>>> + } else {<br>
>>>> + Diag(Tok, diag::warn_pragma_include_alias_expected) << "include<br>
>>>> filename";<br>
>>>> + return;<br>
>>>> + }<br>
>>><br>
>>> Could this repeated code be factored out? (I imagine we have largely the<br>
>>> same pattern in some other places too.)<br>
>>><br>
>>>> +<br>
>>>> + // Finally, we expect the closing paren<br>
>>>> + Lex(Tok);<br>
>>>> + if (Tok.isNot(tok::r_paren)) {<br>
>>>> + Diag(Tok, diag::warn_pragma_include_alias_expected) << ")";<br>
>>>> + return;<br>
>>>> + }<br>
>>>> +<br>
>>>> + // Now that we have the source and target filenames, we need to make<br>
>>>> sure<br>
>>>> + // they're both of the same type (angled vs non-angled)<br>
>>>> + bool SourceIsAngled =<br>
>>>> + GetIncludeFilenameSpelling(SourceFilenameTok.getLocation(),<br>
>>>> + SourceFileName);<br>
>>>> + bool ReplaceIsAngled =<br>
>>>> + GetIncludeFilenameSpelling(ReplaceFilenameTok.getLocation(),<br>
>>>> + ReplaceFileName);<br>
>>>> + if (SourceIsAngled != ReplaceIsAngled) {<br>
>>>> + Diag(Loc, diag::warn_pragma_include_alias_mismatch);<br>
>>>> + return;<br>
>>>> + }<br>
>>>> +<br>
>>>> + // Now we can let the include handler know about this mapping<br>
>>>> + getHeaderSearchInfo().AddHeaderMapping(SourceFileName,<br>
>>>> ReplaceFileName,<br>
>>><br>
>>> ReplaceFileName here refers to a string which expires at the end of this<br>
>>> function, and it's being saved away by AddHeaderMapping for later use.<br>
>>><br>
>>>> + SourceIsAngled);<br>
>>>> +}<br>
>>>> +<br>
>>>> /// AddPragmaHandler - Add the specified pragma handler to the<br>
>>>> preprocessor.<br>
>>>> /// If 'Namespace' is non-null, then it is a token required to exist on<br>
>>>> the<br>
>>>> /// pragma line before the pragma string starts, e.g. "STDC" or "GCC".<br>
>>>> @@ -943,6 +1038,15 @@<br>
>>>> }<br>
>>>> };<br>
>>>><br>
>>>> +/// PragmaIncludeAliasHandler - "#pragma include_alias("...")".<br>
>>>> +struct PragmaIncludeAliasHandler : public PragmaHandler {<br>
>>>> + PragmaIncludeAliasHandler() : PragmaHandler("include_alias") {}<br>
>>>> + virtual void HandlePragma(Preprocessor &PP, PragmaIntroducerKind<br>
>>>> Introducer,<br>
>>>> + Token &IncludeAliasTok) {<br>
>>>> + PP.HandlePragmaIncludeAlias(IncludeAliasTok);<br>
>>>> + }<br>
>>>> +};<br>
>>>> +<br>
>>>> /// PragmaMessageHandler - "#pragma message("...")".<br>
>>>> struct PragmaMessageHandler : public PragmaHandler {<br>
>>>> PragmaMessageHandler() : PragmaHandler("message") {}<br>
>>>> @@ -1095,5 +1199,6 @@<br>
>>>> // MS extensions.<br>
>>>> if (Features.MicrosoftExt) {<br>
>>>> AddPragmaHandler(new PragmaCommentHandler());<br>
>>>> + AddPragmaHandler(new PragmaIncludeAliasHandler());<br>
>>>> }<br>
>>>> }<br>
>>>> Index: test/Preprocessor/pragma_microsoft.c<br>
>>>> ===================================================================<br>
>>>> --- test/Preprocessor/pragma_microsoft.c (revision 150897)<br>
>>>> +++ test/Preprocessor/pragma_microsoft.c (working copy)<br>
>>>> @@ -38,3 +38,20 @@<br>
>>>> // this warning should go away.<br>
>>>> MACRO_WITH__PRAGMA // expected-warning {{expression result unused}}<br>
>>>> }<br>
>>>> +<br>
>>>> +<br>
>>>> +// This should include macro_arg_directive even though the include<br>
>>>> +// is looking for test.h This allows us to assign to "n"<br>
>>>> +#pragma include_alias("test.h", "macro_arg_directive.h" )<br>
>>>> +#include "test.h"<br>
>>>> +void test( void ) {<br>
>>>> + n = 12;<br>
>>>> +}<br>
>>>> +<br>
>>>> +#pragma include_alias("foo.h", <bar.h>) // expected-warning {{pragma<br>
>>>> include_alias requires matching include directives (e.g<br>
>>>> include_alias("foo.h", "bar.h") or include_alias(<foo.h>, <bar.h>))}}<br>
>>>> +#pragma include_alias("test.h") // expected-warning {{pragma<br>
>>>> include_alias expected ','}}<br>
>>>> +<br>
>>>> +// Make sure that the names match exactly for a replacement, including<br>
>>>> path information. If<br>
>>>> +// this were to fail, we would get a file not found error<br>
>>>> +#pragma include_alias(".\pp-record.h", "does_not_exist.h")<br>
>>>> +#include "pp-record.h"<br>
>>><br>
>><br>
>> Hi Richard -- I've addressed almost all of the points in your review,<br>
>> and have attached a patch for you to check over again. Thank you for<br>
>> all the helpful feedback!<br>
>><br>
>> The only thing I didn't address was:<br>
>><br>
>>> Could this repeated code be factored out? (I imagine we have largely the<br>
>>> same pattern in some other places too.)<br>
>><br>
>> I'm guessing the code could be refactored out, but I'd rather that be<br>
>> a separate patch since it touches on more than just include_alias.<br>
>><br>
>> Thanks!<br>
><br>
> A few additional comments, I suggest optimizing for the common path, that there is no include_alias in the source, so:<br>
><br>
> + typedef llvm::StringMap<std::string, llvm::BumpPtrAllocator><br>
> + IncludeAliasMap;<br>
> + IncludeAliasMap IncludeAliases;<br>
><br>
> Have this be a pointer and create the object lazily on demand:<br>
> OwningPtr<IncludeAliasMap> IncludeAliases;<br>
><br>
> + // Map the filename with the brackets still attached. If the name doesn't<br>
> + // map to anything, fall back on the filename we've already gotten the<br>
> + // spelling for.<br>
> + std::string NewName = HeaderInfo.MapHeader(OriginalFilename);<br>
> + if (!NewName.empty())<br>
> + Filename = NewName;<br>
> +<br>
><br>
> here you can check if the object was created or not before going ahead to call HeaderInfo.MapHeader(OriginalFilename).<br>
><br>
> + void AddHeaderMapping(StringRef Source, StringRef Dest) {<br>
> and<br>
> + std::string MapHeader(StringRef Source) {<br>
><br>
> Rename to "AddIncludeAlias" and "MapHeaderToIncludeAlias" to make clear what these are for ?<br>
<br>
</div></div>Thanks for the reviews -- I've attached a patch with your suggestions<br>
incorporated. I've cleaned up the items Richard pointed out, and made<br>
the alias mapping lazy-loaded as Argyrios suggested.<br></blockquote><div><br></div><div>Minor whitespacey things:</div><div><br></div><div><div>+ // Do any filename replacements before anything else</div><div>+ IncludeAliasMap::const_iterator Iter = </div>
<div>+ IncludeAliases->find(Source);</div><div><br></div><div>This will fit on a single line.</div><div><br></div><div>+ if (Iter != IncludeAliases->end())</div><div>+ return Iter->second;</div></div>
<div><br></div><div>This is indented too far.</div><div><br></div><div>Other than that, you've addressed all of my concerns, but please don't commit until Argyrios is happy too.</div></div>