[cfe-commits] [PATCH] Analysis/CFG Build options
Marcin Świderski
marcin.sfider at gmail.com
Tue Sep 14 10:41:12 PDT 2010
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>
More information about the cfe-commits
mailing list