[cfe-commits] [PATCH] Support for pragma include_alias
Aaron Ballman
aaron at aaronballman.com
Fri Mar 2 09:51:31 PST 2012
On Thu, Mar 1, 2012 at 8:07 PM, Argyrios Kyrtzidis <kyrtzidis at apple.com> wrote:
> 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 ?
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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: include_alias.patch
Type: application/octet-stream
Size: 12282 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120302/24757156/attachment.obj>
More information about the cfe-commits
mailing list