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

[Ted Kremenek]

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;
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20100914/93ea4c02/attachment.html>