r178105 - [modules] Re-enable the "ambiguous expansion of macro" warning.
Richard Smith
richard at metafoo.co.uk
Fri Oct 25 14:44:25 PDT 2013
On Tue, Mar 26, 2013 at 6:25 PM, Argyrios Kyrtzidis <akyrtzi at gmail.com>wrote:
> Author: akirtzidis
> Date: Tue Mar 26 20:25:19 2013
> New Revision: 178105
>
> URL: http://llvm.org/viewvc/llvm-project?rev=178105&view=rev
> Log:
> [modules] Re-enable the "ambiguous expansion of macro" warning.
>
> Also update "test/Modules/macros.c" to test modified semantics:
> -When there is an ambiguous macro, expand using the latest introduced
> version, not the first one.
> -#undefs in submodules cause the macro to not be exported by that
> submodule, it doesn't cause
> undefining of macros in the translation unit that imported that submodule.
> This reduces macro namespace interference across modules.
>
> Modified:
> cfe/trunk/lib/Lex/PPMacroExpansion.cpp
> cfe/trunk/test/Modules/macros.c
>
> Modified: cfe/trunk/lib/Lex/PPMacroExpansion.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Lex/PPMacroExpansion.cpp?rev=178105&r1=178104&r2=178105&view=diff
>
> ==============================================================================
> --- cfe/trunk/lib/Lex/PPMacroExpansion.cpp (original)
> +++ cfe/trunk/lib/Lex/PPMacroExpansion.cpp Tue Mar 26 20:25:19 2013
> @@ -211,7 +211,9 @@ bool Preprocessor::isNextPPTokenLParen()
> /// expanded as a macro, handle it and return the next token as
> 'Identifier'.
> bool Preprocessor::HandleMacroExpandedIdentifier(Token &Identifier,
> MacroDirective *MD) {
> - MacroInfo *MI = MD->getMacroInfo();
> + MacroDirective::DefInfo Def = MD->getDefinition();
> + assert(Def.isValid());
> + MacroInfo *MI = Def.getMacroInfo();
>
> // If this is a macro expansion in the "#if !defined(x)" line for the
> file,
> // then the macro could expand to different things in other contexts,
> we need
> @@ -286,25 +288,22 @@ bool Preprocessor::HandleMacroExpandedId
> }
> }
>
> - // FIXME: Temporarily disable this warning that is currently bogus with
> a PCH
> - // that redefined a macro without undef'ing it first
> (test/PCH/macro-redef.c).
> -#if 0
> // If the macro definition is ambiguous, complain.
> - if (MI->isAmbiguous()) {
> + if (Def.getDirective()->isAmbiguous()) {
> Diag(Identifier, diag::warn_pp_ambiguous_macro)
> << Identifier.getIdentifierInfo();
> Diag(MI->getDefinitionLoc(), diag::note_pp_ambiguous_macro_chosen)
> << Identifier.getIdentifierInfo();
> - for (MacroInfo *PrevMI = MI->getPreviousDefinition();
> - PrevMI && PrevMI->isDefined();
> - PrevMI = PrevMI->getPreviousDefinition()) {
> - if (PrevMI->isAmbiguous()) {
> - Diag(PrevMI->getDefinitionLoc(),
> diag::note_pp_ambiguous_macro_other)
> + for (MacroDirective::DefInfo PrevDef = Def.getPreviousDefinition();
> + PrevDef && !PrevDef.isUndefined();
> + PrevDef = PrevDef.getPreviousDefinition()) {
> + if (PrevDef.getDirective()->isAmbiguous()) {
> + Diag(PrevDef.getMacroInfo()->getDefinitionLoc(),
> + diag::note_pp_ambiguous_macro_other)
> << Identifier.getIdentifierInfo();
> }
> }
> }
> -#endif
>
> // If we started lexing a macro, enter the macro expansion body.
>
>
> Modified: cfe/trunk/test/Modules/macros.c
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/macros.c?rev=178105&r1=178104&r2=178105&view=diff
>
> ==============================================================================
> --- cfe/trunk/test/Modules/macros.c (original)
> +++ cfe/trunk/test/Modules/macros.c Tue Mar 26 20:25:19 2013
> @@ -1,4 +1,3 @@
> -// XFAIL: *
> // RUN: rm -rf %t
> // RUN: %clang_cc1 -fmodules -x objective-c -emit-module
> -fmodules-cache-path=%t -fmodule-name=macros_top %S/Inputs/module.map
> // RUN: %clang_cc1 -fmodules -x objective-c -emit-module
> -fmodules-cache-path=%t -fmodule-name=macros_left %S/Inputs/module.map
> @@ -10,12 +9,12 @@
> // These notes come from headers in modules, and are bogus.
>
> // FIXME: expected-note{{previous definition is here}}
> +// FIXME: expected-note{{previous definition is here}}
> expected-note{{expanding this definition of 'LEFT_RIGHT_DIFFERENT'}}
> +// expected-note{{other definition of 'TOP_RIGHT_REDEF'}}
> expected-note{{expanding this definition of 'LEFT_RIGHT_DIFFERENT2'}}
> // expected-note{{other definition of 'LEFT_RIGHT_DIFFERENT'}}
> -// expected-note{{expanding this definition of 'TOP_RIGHT_REDEF'}}
> -// FIXME: expected-note{{previous definition is here}} \
> -// expected-note{{expanding this definition of 'LEFT_RIGHT_DIFFERENT'}}
>
> -// expected-note{{other definition of 'TOP_RIGHT_REDEF'}}
> +
> +// expected-note{{expanding this definition of 'TOP_RIGHT_REDEF'}}
>
> @import macros;
>
> @@ -80,8 +79,8 @@ void f() {
> # error TOP should be visible
> #endif
>
> -#ifdef TOP_LEFT_UNDEF
> -# error TOP_LEFT_UNDEF should not be visible
> +#ifndef TOP_LEFT_UNDEF
> +# error TOP_LEFT_UNDEF should still be defined
>
This seems completely broken to me, and it breaks libc++.
glibc has something like:
// stdio.h
int getc(FILE*);
#define foo_getc(x) /* ... */
#define getc(x) foo_getc(x)
libc++ has:
// cstdio
#include <stdio.h>
#ifdef getc
#undef getc
#endif
namespace std { using ::getc; }
This results in any program which includes <cstdio> being unable to use
std::getc:
// main.cc
#include <cstdio>
int k = std::getc(stdin); // error, no foo_getc in namespace std
Can you explain a bit about the effect you were trying to achieve here?
Here's another case:
#include <stdio.h>
#include <something>
int k = getc(stdin); // still uses the macro from <stdio.h>
I think we might want to ignore an #undef in <something> if <something>
comes from a module that doesn't depend on the module containing <stdio.h>.
(If it's just using the macro for its own internal purposes, or more
generally if rearranging the order in which we chose to load the modules
would result in a consistent macro definition chain.)
I think the behavior we want here is:
* If a module M1 depends on a module M2, macro definitions/undefs in M1 and
M2 are never ambiguous (M1 is always ordered after M2).
* If there are multiple ambiguous 'most recent' definitions of a macro
(excluding chains that end in an undef), and the macro is used, warn.
Put another way: consider the subgraph of the imported portion of the
module dependency graph in which the macro is or was defined. All roots of
that subgraph for which the macro is defined must define it to the same
value, or we warn on ambiguity. If there are any such roots, the value of
the macro is the value at such a root. Otherwise, the macro is not defined.
Does that sound right to you?
#endif
>
> void test1() {
> @@ -89,7 +88,8 @@ void test1() {
> TOP_RIGHT_REDEF *ip = &i;
> }
>
> -#define LEFT_RIGHT_DIFFERENT2 double // FIXME:
> expected-warning{{'LEFT_RIGHT_DIFFERENT2' macro redefined}}
> +#define LEFT_RIGHT_DIFFERENT2 double // FIXME:
> expected-warning{{'LEFT_RIGHT_DIFFERENT2' macro redefined}} \
> + // expected-note{{other definition
> of 'LEFT_RIGHT_DIFFERENT2'}}
>
> // Import right module (which also imports top)
> @import macros_right;
> @@ -112,11 +112,11 @@ void test2() {
> int i;
> float f;
> double d;
> - TOP_RIGHT_REDEF *ip = &i; // expected-warning{{ambiguous expansion of
> macro 'TOP_RIGHT_REDEF'}}
> + TOP_RIGHT_REDEF *fp = &f; // expected-warning{{ambiguous expansion of
> macro 'TOP_RIGHT_REDEF'}}
>
> - LEFT_RIGHT_IDENTICAL *ip2 = &i;
> - LEFT_RIGHT_DIFFERENT *fp = &f; // expected-warning{{ambiguous expansion
> of macro 'LEFT_RIGHT_DIFFERENT'}}
> - LEFT_RIGHT_DIFFERENT2 *dp = &d;
> + LEFT_RIGHT_IDENTICAL *ip = &i;
> + LEFT_RIGHT_DIFFERENT *ip2 = &i; // expected-warning{{ambiguous
> expansion of macro 'LEFT_RIGHT_DIFFERENT'}}
> + LEFT_RIGHT_DIFFERENT2 *ip3 = &i; // expected-warning{{ambiguous
> expansion of macro 'LEFT_RIGHT_DIFFERENT2}}
> int LEFT_RIGHT_DIFFERENT3;
> }
>
> @@ -133,6 +133,6 @@ void test3() {
>
> @import macros_right.undef;
>
> -#ifdef TOP_RIGHT_UNDEF
> -# error TOP_RIGHT_UNDEF should not be defined
> +#ifndef TOP_RIGHT_UNDEF
> +# error TOP_RIGHT_UNDEF should still be defined
> #endif
>
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20131025/2c58ddca/attachment.html>
More information about the cfe-commits
mailing list