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;_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
