On Oct 25, 2013, at 5:33 PM, Richard Smith <[email protected]> wrote:
> On Fri, Oct 25, 2013 at 4:54 PM, Argyrios Kyrtzidis <[email protected]> wrote: > > 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. > > Who says you imported M2? I'm not sure what writing "@import M2" explicitly means, if not that I imported M2 ? > > If you didn't import M2, you don't want its macros leaking out of M1, > especially since M1 explicitly tried to remove them. > > If you imported both of them, then the effect of importing M1 and M2 is that > you don't get the macro. M1 gets to decide this, because it is higher up in > the layering. This is exactly what would happen with header files. You can do some horrible things with header files, I don't find "this is exactly what would happen with header files" a strong argument; modules are supposed to be more resilient and self-contained when combined together. > >> 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 ? > > Why would you want to? M1 is removing that macro for a reason. I've given you > a concrete example of a case where what I'm proposing is the right thing; I agreed that "@import cstdio" should not export the 'getc' macro > I'd like to see an example of a case where what you're proposing is the right > thing. No-one I've spoken to thinks it makes sense, but perhaps we're missing > something here. > > If you want this effect, use the macro from M2 before you import M1, or in a > different TU. Same as with header files. With your suggestion my issue is that I cannot access the macros that M2 exports, irrespective of whether or not I imported another module; your workaround is that I should restructure my code. With my suggestion your issue is that the macro from M2 affects a symbol with the same name from M1; my workaround is that you simply remove the import of M1 and import only M2 (as the author of M2 intended).
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
