[cfe-commits] [PATCH] Support for pragma include_alias
Aaron Ballman
aaron at aaronballman.com
Thu Mar 1 10:43:09 PST 2012
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!
~Aaron
-------------- next part --------------
A non-text attachment was scrubbed...
Name: include_alias.patch
Type: application/octet-stream
Size: 11685 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120301/349077f0/attachment.obj>
More information about the cfe-commits
mailing list