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

[Sean Silva]

On Wed, Nov 6, 2013 at 5:10 AM, David Majnemer <david.majnemer at gmail.com>wrote:

>
>   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?
>

Besides the general improved clarity in the source code (no more `true,
false, false, true`-disease code), from a calling-convention perspective
(at least on x86 where I'm familiar, but I'm pretty sure it holds for other
architectures), passing N bools is extremely inefficient and will typically
occupy as much of the register set as the passing N ints, and also require
at least N instructions to load up the registers for the call and also on
the callee side to read them, whereas a bitmask occupies one int's worth of
registers and can be loaded efficiently with a single instruction (assuming
all the parameters are constant at compile time, which I think is true in
this case). Also, I don't think that our register allocator is smart enough
to pack bools bitwise into a single register to decrease register pressure
in the callee, and even if it were the extra code to do so would still
present an overhead.

So basically, it's a win-win from both a human and machine perspective.

-- Sean Silva


>
>
> ================
> 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
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20131108/10ec8738/attachment.html>