[llvm-commits] [llvm] r172440 - in /llvm/trunk: include/llvm/MC/MCParser/MCAsmParser.h lib/MC/MCParser/AsmParser.cpp

[Jim Grosbach]

On Feb 20, 2013, at 2:13 PM, Eli Bendersky <eliben at google.com> wrote:

> On Wed, Feb 20, 2013 at 2:10 PM, Jim Grosbach <grosbach at apple.com> wrote:
>> Hmmm… so it is. That's annoying.
>> 
>> Patch incoming….
>> 
>> -j
> 
> Cool, and that earlier commit too. Incidentally, I've been under the
> (perhaps misguided) impression that we prefer not to do these
> mega-coding-convention-rewrite patches on old code. Is this a license
> to kill ;-) ?
> 

There's a bit of judgement call involved. We avoid big huge "update the whole codebase for style/whitespace change XYZ" type commits. Instead we do small(er) "while I'm in there anyway…" updates. Personally, my yardstick is to do up to a class interface at a time for things like this, or a single file at a time for things like updating a pass. Whatever makes sense as a single "this stuff all goes together" chunk, basically.

Also note that I'm a bit more aggressive about these sorts of changes than most, so take the above with a side helping of caution. ;)

-Jim

> 
> 
>> On Feb 20, 2013, at 12:37 PM, Eli Bendersky <eliben at google.com> wrote:
>> 
>>> Hi Rafael,
>>> This patch was eventually rewritten differently so these methods no
>>> longer exist. Generally, however, i try to follow the style in
>>> existing code. Although now when looking at it, the style within
>>> MCAsmParser is already inconsistent :-/
>>> 
>>> Eli
>>> 
>>> 
>>> On Wed, Feb 20, 2013 at 10:28 AM, Rafael Espíndola
>>> <rafael.espindola at gmail.com> wrote:
>>>> Please use start new function names with a lowercase letter:
>>>> 
>>>> http://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly
>>>> 
>>>> On 14 January 2013 13:08, Eli Bendersky <eliben at google.com> wrote:
>>>>> Author: eliben
>>>>> Date: Mon Jan 14 12:08:41 2013
>>>>> New Revision: 172440
>>>>> 
>>>>> URL: http://llvm.org/viewvc/llvm-project?rev=172440&view=rev
>>>>> Log:
>>>>> Encapsulate the MacroEnabled flag in AsmParser behind accessor methods.
>>>>> 
>>>>> The methods are also exposed via the MCAsmParser interface, which allows more
>>>>> than one client to control them. Previously, GenericAsmParser was playing with
>>>>> a member var in AsmParser directly (by virtue of being its friend).
>>>>> 
>>>>> 
>>>>> Modified:
>>>>>   llvm/trunk/include/llvm/MC/MCParser/MCAsmParser.h
>>>>>   llvm/trunk/lib/MC/MCParser/AsmParser.cpp
>>>>> 
>>>>> Modified: llvm/trunk/include/llvm/MC/MCParser/MCAsmParser.h
>>>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/MC/MCParser/MCAsmParser.h?rev=172440&r1=172439&r2=172440&view=diff
>>>>> ==============================================================================
>>>>> --- llvm/trunk/include/llvm/MC/MCParser/MCAsmParser.h (original)
>>>>> +++ llvm/trunk/include/llvm/MC/MCParser/MCAsmParser.h Mon Jan 14 12:08:41 2013
>>>>> @@ -136,6 +136,10 @@
>>>>>  /// recovery.
>>>>>  virtual void EatToEndOfStatement() = 0;
>>>>> 
>>>>> +  /// Control a flag in the parser that enables or disables macros.
>>>>> +  virtual bool MacrosEnabled() = 0;
>>>>> +  virtual void SetMacrosEnabled(bool flag) = 0;
>>>>> +
>>>>>  /// ParseExpression - Parse an arbitrary expression.
>>>>>  ///
>>>>>  /// @param Res - The value of the expression. The result is undefined
>>>>> 
>>>>> Modified: llvm/trunk/lib/MC/MCParser/AsmParser.cpp
>>>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/MC/MCParser/AsmParser.cpp?rev=172440&r1=172439&r2=172440&view=diff
>>>>> ==============================================================================
>>>>> --- llvm/trunk/lib/MC/MCParser/AsmParser.cpp (original)
>>>>> +++ llvm/trunk/lib/MC/MCParser/AsmParser.cpp Mon Jan 14 12:08:41 2013
>>>>> @@ -151,7 +151,7 @@
>>>>>  std::vector<MacroInstantiation*> ActiveMacros;
>>>>> 
>>>>>  /// Boolean tracking whether macro substitution is enabled.
>>>>> -  unsigned MacrosEnabled : 1;
>>>>> +  unsigned MacrosEnabledFlag : 1;
>>>>> 
>>>>>  /// Flag tracking whether any errors have been encountered.
>>>>>  unsigned HadError : 1;
>>>>> @@ -231,6 +231,9 @@
>>>>>  virtual bool ParseIdentifier(StringRef &Res);
>>>>>  virtual void EatToEndOfStatement();
>>>>> 
>>>>> +  virtual bool MacrosEnabled() {return MacrosEnabledFlag;}
>>>>> +  virtual void SetMacrosEnabled(bool flag) {MacrosEnabledFlag = flag;}
>>>>> +
>>>>>  /// }
>>>>> 
>>>>> private:
>>>>> @@ -503,7 +506,7 @@
>>>>>                     MCStreamer &_Out, const MCAsmInfo &_MAI)
>>>>>  : Lexer(_MAI), Ctx(_Ctx), Out(_Out), MAI(_MAI), SrcMgr(_SM),
>>>>>    GenericParser(new GenericAsmParser), PlatformParser(0),
>>>>> -    CurBuffer(0), MacrosEnabled(true), CppHashLineNumber(0),
>>>>> +    CurBuffer(0), MacrosEnabledFlag(true), CppHashLineNumber(0),
>>>>>    AssemblerDialect(~0U), IsDarwin(false), ParsingInlineAsm(false) {
>>>>>  // Save the old handler.
>>>>>  SavedDiagHandler = SrcMgr.getDiagHandler();
>>>>> @@ -1278,7 +1281,7 @@
>>>>>  }
>>>>> 
>>>>>  // If macros are enabled, check to see if this is a macro instantiation.
>>>>> -  if (MacrosEnabled)
>>>>> +  if (MacrosEnabled())
>>>>>    if (const Macro *M = MacroMap.lookup(IDVal))
>>>>>      return HandleMacroEntry(IDVal, IDLoc, M);
>>>>> 
>>>>> @@ -3489,7 +3492,7 @@
>>>>>    return Error(getLexer().getLoc(),
>>>>>                 "unexpected token in '" + Directive + "' directive");
>>>>> 
>>>>> -  getParser().MacrosEnabled = Directive == ".macros_on";
>>>>> +  getParser().SetMacrosEnabled(Directive == ".macros_on");
>>>>> 
>>>>>  return false;
>>>>> }
>>>>> 
>>>>> 
>>>>> _______________________________________________
>>>>> llvm-commits mailing list
>>>>> llvm-commits at cs.uiuc.edu
>>>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>> 
>>> _______________________________________________
>>> llvm-commits mailing list
>>> llvm-commits at cs.uiuc.edu
>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>