[cfe-dev] CFGElement changes and initializers addition (with patch)

[Marcin Świderski]

Hi Jordy, Ted

Thanks for the appreciation and for comments. Some comments inline:

W dniu 18 sierpnia 2010 07:47 użytkownik Jordy Rose
<jediknil at belkadan.com>napisał:

>
> Wow, good work.
>
> I agree with most of what Ted said. A few additional comments:
>
> - I like the single enum rather than kind and subkind, but there are 7
> elements, not 6. So it won't fit in a PointerIntPair anyway, though having
> a Stmt* field (or void*) and an enum field would be preferable to two
> PointerIntPairs.
> I'm still not totally convinced we need scope markers in the CFG, though.
>
> For both destructor kinds I will need space for two pointers. For automatic
object I need it's declaration and statement that cause destructor to be
called. For temporary object I need expression bound to the temporary and
the full expression that contains the temporary.

I could allocate separate struct for those and point to it from CFGElement.
If we drop the scopes and move information about kind of destructor to this
struct, then CFGElement will need only one PointerIntPair.

If we would want scopes modeled we could use same struct as for destructors.
So we would end up with something like this:

- Statement and LValStatement with pointer to Stmt*,
- Initializer with pointer to CXXBaseOrMemberInitializer*,
- Extendend CFGElement with pointer to struct with information about
destructor or scope.

Our kind enum would look like this:
enum Kind {
  Statement,
  LvalStatment,
  BEGIN_STATEMENT = Statement,
  END_STATEMENT = LvalStatement,
  Initializer,

  ExtendedElement, // This would be internal, client would never get this
                              // with getKind() method.

  AutomaticObjectDtor,
  TemporaryObjectDtor,
  BEGIN_DTOR = AutomaticObjectDtor,
  END_DTOR = TemporaryObjectDtor,
  StartScope,
  EndScope,
  BEGIN_SCOPE = StartScope,
  END_SCOPE = EndScope
};

This way we will have space efficient CFG for Obj-C and C languages. For C++
CFG will occupy less memory if there will be more statement then destructor
CFGElements.

- Why don't we make this a static type hierarchy like SVal or MemRegion?
> We'd have CFGStmt, CFGInitializer, CFGDestructor, and CFGScope(Marker)
> inheriting from CFGElement. That way we can have, among other things, a
> CFGStmt that simplifies to a Stmt. Instead of asserts, we can just have the
> type-specific methods on the subclasses.
>
> Yes, this will be much better approach. I will change this.

- In UnreachableCodeChecker::getUnreachableStmt(), isn't the block
> guaranteed to have an initial statement for the same reason as in
> ReachableCode.cpp?
>
> Do you mean that if block is unreachable it will start with a statement and
not with initializer, destructor or scope?


> - Very tiny comment: most of the LLVM codebase has no indentation on blank
> lines.
>
> I will try to follow this convention then.


> Great to get more C++ support in the analyzer!
> Jordy
>

Cheers,
Marcin
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20100818/92fc35e9/attachment.html>