Hi, Anna. Some comments:
> (I am not sure if we should move the setters and getters as well and
> make them into static methods..)
I can see both sides of this. On the one hand, these methods are not related to
the workings of ProgramState; they are shared auxiliary data. But on the other,
CallEvent is definitely part of Core, and getDynamicTypeInfo is /not/ just a
simple GDM getter -- it will infer a type from the region being passed if no
type info has been set. So I'm not really going to push one way or the other.
> +/// \class DynamicTypeInfo
> +///
We generally don't include this in our Doxygen comments; "don't repeat
yourself". (If the name ever changes, someone will forget to change the
comment.)
> +class DynamicTypeInfo {
> +public:
> +
> +private:
Empty public section?
> + /// \brief Return true if no dynamic type info is available.
> + bool isValid() const { return !T.isNull(); }
Returns true if type info IS available, not if it isn't. Also, I'd omit the
word "dynamic" here; not only is it sort of implicit in the context, but a lot
of times DynamicTypeInfo objects will be no better than the static type
information anyway, and one could argue that "no better than static" is the
same as "no dynamic info is available".
> + /// \brief Returns false if the type T is the only type in the lattice
> + /// (the type information is precise), true otherwise.
> + bool canBeASubClass() const { return CanBeASubClass; }
This is the only time we ever use the word "lattice". Yes, C++ type inheritance
is a DAG, but that's not a concept we've used in the analyzer until now. TBH,
the name 'canBeASubclass' tells me more about when the function returns true
than the comment.
> + void Profile(llvm::FoldingSetNodeID &ID) const {
> + T.Profile(ID);
> + ID.AddInteger((unsigned)CanBeASubClass);
> + }
I think the correct way to do this is
ID.Add(T);
ID.AddBoolean(CanBeASubClass);
> +}} // end clang::ento namespace
The comment doesn't quite match here; "clang::ento" is still only a single
namespace. More generally, we haven't really stacked braces until recently. Are
we going to do this for all nested namespaces moving forwards? If not, I'd
rather stick with the current convention of two lines.
Jordan
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
