[PATCH] Replaced bool parameters in SkipUntil function with single bit-based parameter.

Hal Finkel hfinkel at anl.gov
Wed Nov 6 06:29:39 PST 2013


To be specific, I suggested this approach in review for http://llvm-reviews.chandlerc.com/D1847 -- which has several changes like this:

@@ -179,2 +180,3 @@
+    SkipUntil(tok::annot_pragma_openmp_end, false, false, false, true);
     break;
   }

and I consider this fairly unreadable. I think that there is some cutoff between for how many parameters it is reasonable to add /* ArgName = */ comments, and when we should switch to or'd flags. Personally, I think that at 4 flags, the bit field is a better way (because it is more self-documenting).

 -Hal

----- Original Message -----
> 
> Hi David,
> I need to add another one boolean parameter for OpenMP pragmas
> parsing
> to skip counting braces/brackets/parens and Hal Finkel proposed to
> convert all these booleans into a bit-based single parameter.
> 
> Best regards,
> Alexey Bataev
> =============
> Software Engineer
> Intel Compiler Team
> Intel Corp.
> 
> > Date: Wed, 6 Nov 2013 14:24:26 +0400
> > From: a.bataev at hotmail.com
> > To:
> > reviews+D2108+public+7ef5c38906feb203 at llvm-reviews.chandlerc.com;
> > dgregor at apple.com; cbergstrom at pathscale.com; hfinkel at anl.gov
> > CC: cfe-commits at cs.uiuc.edu; david.majnemer at gmail.com
> > Subject: Re: [PATCH] Replaced bool parameters in SkipUntil function
> > with single bit-based parameter.
> > 
> > Hi David,
> > I need to add another one boolean parameter for OpenMP pragmas
> > parsing
> > to skip counting braces/brackets/parens and Hal Finkel proposed to
> > convert all these booleans into a bit-based single parameter.
> > 
> > Best regards,
> > Alexey Bataev
> > =============
> > Software Engineer
> > Intel Compiler Team
> > Intel Corp.
> > 
> > 06.11.2013 14:10, David Majnemer пишет:
> > > From my experience,
> > > `SkipUntil(EndKind, /*StopAtSemi=*/true, /*DontConsume=*/true);`
> > > is more typical in clang than
> > > `SkipUntil(EndKind, StopAtSemi | DontConsume);`
> > > 
> > > It seems that some of the calls that you changed were previously
> > > nasty (i.e. `SkipUntil(EndKind, true, true)`) which justifies a
> > > cleanup.
> > > However I'm not sure we want a bitfield here.
> > > 
> > > What is your justification?
> > > 
> > > 
> > > ================
> > > Comment at: include/clang/Parse/Parser.h:751
> > > @@ -741,3 +750,3 @@
> > > /// token will ever occur, this skips to the next token, or to
> > > some likely
> > > - /// good stopping point. If StopAtSemi is true, skipping will
> > > stop at a ';'
> > > - /// character.
> > > + /// good stopping point. If Flags has bit set at StopAtSemi,
> > > skipping will
> > > + /// stop at a ';' character.
> > > ----------------
> > > The wording "has bit set" seems strange.
> > > 
> > > 
> > > http://llvm-reviews.chandlerc.com/D2108
> > 
> 

-- 
Hal Finkel
Assistant Computational Scientist
Leadership Computing Facility
Argonne National Laboratory




More information about the cfe-commits mailing list