[cfe-commits] [PATCH] Analysis/CFG Build options

[Marcin Świderski]

W dniu 14 września 2010 18:14 użytkownik Ted Kremenek <
kremenek at cs.stanford.edu> napisał:

>
> On Sep 14, 2010, at 9:05 AM, Ted Kremenek wrote:
>
>
> On Sep 14, 2010, at 12:07 AM, Marcin Świderski wrote:
>
> Hi
>
>
> This patch implements named parameter idiom for CFG::buildCFG options.
>
>
> Cheers,
>
> Marcin
>
>
> PS. I can't see messages I send on cfe-commits. Should I send patches to
> cfe-dev?
>
> <cfg-build-options.patch>
>
>
> Looks great.  Minor nit:
>
> +    bool PruneTriviallyFalseEdges:1;
> +    bool AddEHEdges:1;
> +    bool AddInitializers:1;
> +    bool AddImplicitDtors:1;
>
> These probably need to be 'unsigned' instead of 'bool'.  I vaguely recall
> bool bitfields to be a problem for Visual C++ (treats them as signed?).  The
> Clang codebase seems inconsistent, so I'm going to ask a question on
> cfe-dev.
>
>
> On second thought, I think this is fine, since the issue was with enum
> bitfields, not bool.
>
> One other comment:
>
> +  class BuildOptions {
> +  public:
> +    bool PruneTriviallyFalseEdges:1;
> +    bool AddEHEdges:1;
> +    bool AddInitializers:1;
> +    bool AddImplicitDtors:1;
> +
> +    BuildOptions()
> +        : PruneTriviallyFalseEdges(true)
> +        , AddEHEdges(false)
> +        , AddInitializers(false)
> +        , AddImplicitDtors(false) {}
> +    BuildOptions setPruneTriviallyFalseEdges(bool v) {
> +      PruneTriviallyFalseEdges = v;
> +      return *this;
> +    }
> +    BuildOptions setAddEHEdges(bool v) {
> +      AddEHEdges = v;
> +      return *this;
> +    }
> +    BuildOptions setAddInitializers(bool v) {
> +      AddInitializers = v;
> +      return *this;
> +    }
> +    BuildOptions setAddImplicitDtors(bool v) {
> +      AddImplicitDtors = v;
> +      return *this;
> +    }
> +  };
>
>
> If the bool fields are public, why have the setters?  While the following
> is elegant:
>
> +        CFG::BuildOptions().setPruneTriviallyFalseEdges(false)
> +        .setAddEHEdges(AddEHEdges));
>
> It's probably clearer just to write this as:
>
> CFG::BuildOptions B;
> B.PruneTriviallyFalseEdges = false;
> B.AddEHEdges = AddEHEdges;
>

I'm used to using named parameters object as temporary created in function's
parameters list, just like I did in AnalysisContext.cpp . Such approach is
clearer IMO, because it shows exactly what I want to achieve: pass
parameters to function, and not create an object named B and pass it to
function.

But that's just my opinion and I can remove those setters if it will better
fit the whole project.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20100914/a5b3f8cc/attachment.html>