[cfe-commits] [PATCH] Support for pragma include_alias
Argyrios Kyrtzidis
kyrtzidis at apple.com
Thu Mar 1 18:07:39 PST 2012
On Mar 1, 2012, at 10:43 AM, Aaron Ballman wrote:
> On Thu, Mar 1, 2012 at 12:21 AM, Richard Smith <richard at metafoo.co.uk> wrote:
>> Hi,
>>
>>>> On Sun, Feb 19, 2012 at 5:31 PM, Aaron Ballman <aaron at aaronballman.com>
>>>> 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 ?
-Argyrios
>
> ~Aaron
> <include_alias.patch>_______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
More information about the cfe-commits
mailing list