[cfe-dev] [PATCH] preprocessor pragma push_macro support

[Francois Pichet]

Any reason why this has not been committed?

On Sun, Aug 1, 2010 at 3:48 AM, Francois Pichet <pichet2000 at gmail.com> wrote:
> Please ignore the latest patch it contains a major bug.
> Here is the new (hopefully) bug free patch.
>
>
>
> On Sun, Aug 1, 2010 at 1:29 AM, Francois Pichet <pichet2000 at gmail.com> wrote:
>> Hi
>>
>> This is a new patch after rework based on your comments.
>>
>>
>> On Fri, Jul 30, 2010 at 1:18 PM, Chris Lattner <clattner at apple.com> wrote:
>>>
>>> On Jul 28, 2010, at 3:44 AM, Francois Pichet wrote:
>>>
>>>> This is the patch after rework.
>>>> The only thing I didn't implement is to remove the assert. User code
>>>> cannot trigger the assert (I believe), the check on line 503 will
>>>> prevent it:
>>>>    if (Tok.isNot(tok::string_literal)) {
>>>>
>>>> Comments greatly appreciated.
>>>
>>> Thanks for working on this!  The patch is looking good, but I have a few more minor comments:
>>>
>>> +  /// IsPragmaPush - True if this macro has been pushed using #pragma push_macro
>>> +  /// This is used to prevent releasing the macro.
>>> +  bool IsPragmaPush : 1;
>>>
>>> Your implementation approach introduces sharing of the MacroInfo object: the identifier info table can have one pointer and the pragma push_macro stack can have another.  I think it would be cleaner to just *make a copy* of the MacroInfo object on pragma push, to avoid this sharing.  This is slightly less efficient than your solution, but simplifies the code, and push_macro hopefully shouldn't be a performance bottleneck :)
>>>
>>>
>>> +  /// isPragmaPush - Return true if this macro is on the #pragma push_macro stack.
>>> +  bool isPragmaPush() const { return IsPragmaPush; }
>>>
>>> Please wrap to 80 columns.
>>>
>>> +  /// PragmaPushMacroInfo - For each IdentifierInfo used in a #pragma
>>> +  /// push_macro directive, we keep a MacroInfo stack used to restore
>>> +  /// previous macro value.
>>> +  std::map<IdentifierInfo*, std::stack<MacroInfo*> > PragmaPushMacroInfo;
>>>
>>> Please use
>>>  llvm::DenseMap<IdentifierInfo*, std::vector<MacroInfo*> > PragmaPushMacroInfo;
>>>
>>> for consistency with our containers.
>>>
>>>
>>> +  IdentifierInfo* ParsePragmaPushOrPopMacro(Token &Tok);
>>>
>>> Space before the * :)
>>>
>>>
>>>       // Macros must be identical.  This means all tokes and whitespace
>>>
>>> Not your bug but tokes -> tokens.
>>>
>>> +      // Also do not warn if this is the first redefinition after #pragma
>>> +      // push_macro.
>>> +      if (!MI->isIdenticalTo(*OtherMI, *this) && !OtherMI->isPragmaPush()) {
>>>
>>> Ah.  This is an interesting and rather unfortunate rule.  With the implementation approach suggested above, I think it would make sense to keep a bit in macroinfo, but the bit would be "AllowRedefinitionsWithoutWarning" or something like that.  To me, the semantics of this bit are much more clear and possible more reusable than the 'ispragmapush' bit.
>>>
>>>
>>>
>>> +IdentifierInfo* Preprocessor::ParsePragmaPushOrPopMacro(Token &Tok) {
>>>
>>> Space before the *, not after :)
>>>
>>> +void Preprocessor::HandlePragmaPopMacro(Token &PopMacroTok) {
>>> +  SourceLocation MessageLoc = PopMacroTok.getLocation();
>>> +
>>> +  // Parse the pragma directive and get the macro IdentifierInfo*.
>>> +  IdentifierInfo *IdentInfo = ParsePragmaPushOrPopMacro(PopMacroTok);
>>> +  if (!IdentInfo) return;
>>> +
>>> +  // Retrived the stack<MacroInfo*> associated with the macro.
>>> +  std::map<IdentifierInfo*, std::stack<MacroInfo*> >::iterator iter =
>>> +    PragmaPushMacroInfo.find(IdentInfo);
>>> +  if (iter != PragmaPushMacroInfo.end()) {
>>> +    // PushedMI is the MacroInfo will want to reinstall.
>>>
>>> if iter is end(), then you need to emit the warning about 'no macro_push', right?
>>>
>>> Otherwise, the patch looks great, nice testcase!
>>>
>>> -Chris
>>>
>>>
>>>
>>>
>>>
>>
>