On Oct 25, 2013, at 4:35 PM, Richard Smith <[email protected]> wrote:
> On Fri, Oct 25, 2013 at 4:25 PM, Argyrios Kyrtzidis <[email protected]> wrote: > On Oct 25, 2013, at 2:44 PM, Richard Smith <[email protected]> wrote: > >> On Tue, Mar 26, 2013 at 6:25 PM, Argyrios Kyrtzidis <[email protected]> >> 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? > > I agree that for > > @import cstdio > > 'cstdio' should be able to "not export" the 'getc' macro. > Without some language support to explicitly state whether you are undef'ing > to avoid exporting vs undef'ing for internal use, I think it's reasonable to > choose the former as default (undef'ing to avoid exporting). > > I'm not so sure about M1 just disabling macros from M2, even when I have > explicitly imported M2. I don't like that a module may affect another module > import in a surprising way; for example: > > In my suggestion, this would only happen if both: > 1) M1 depends on M2, so it's undefining the macro defined by M2 not some > other macro that happens to have the same name, and > 2) The undef of the macro is visible (you imported it into this TU). I imported M2 to use its macro; I also imported M1 to use some other symbol from it. It should not be my concern that M1 wants to undef the macro that I want to import, if I don't want to see the macro from M2 I will remove its import declaration. > > I don't think this applies to the situations you want to block; I think my > proposal gives you want you want there. How am I going to be able to use the macro from M2 while still importing M1 ? > > (somewhere in a stack of headers getting included..) > > @import M1 // this undefs macro BAZOO from module M2. > > (...more headers). > > // main.cc > @import M2 // imported so I can use BAZOO from module M2 > BAZOO(0); // undefined ?? > > > I imported M2 to use the macro that it exports, I should not need to know > that M1 depends on M2 and it decided to undef macros from it. > If BAZOO macro is interfering with the usage of a symbol BAZOO from M1, then > M1 should re-export M2 sans BAZOO, so I can use them both by just importing > M1(like cstdio). > > I mean: > > 1) > @import M1 // this re-exports M2 and re-purposes BAZOO; I can use symbols > from both as M1 intended > > > 2) > @import M1 > @import M2 // I imported M2 and expect to be able to use the macros it > exports, including BAZOO > > 3) > @import M2 > @import M1 > // same as 2) > > 4) > @import M1 > @import M2 > #undef BAZOO // I don't like that M2 exports BAZOO macro, I can choose to > undef it. > > > Otherwise, with the suggested model of M1 disabling macros from M2: > > 5) > @import M1 > @import M2 > // There is no way to use symbols from both M1 and M2, and still get access > to the BAZOO that M2 "advertises" that it is exporting. > > >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
