[cfe-commits] r162528: [analyzer] Move DynamicTypeInfo out of the ProgramState.h

[Jordan Rose]

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