[PATCH] D13731: [RFC][Analyzer] Supporting function attributes in .model files.

[pierre gousseau via cfe-commits]

pgousseau added a comment.

In http://reviews.llvm.org/D13731#267905, @xazax.hun wrote:

> Hi!


Hi! Thank you for reviewing.

> I have some high level questions and notes about this patch.

> 

> I implemented the function modelling as a Google Summer of Code project and Ted Kremenek was my mentor. I am happy that you found an useful application of the function modeling system, and I think, in general, it is a good idea to be able to store attributes for the modelled functions. However I am a bit surprised, when I saw this patch. The main reason is that, models lack a lot of functionality right now.

> 

> Main missing features:

> 

> - Ability to specify multiple model paths similar to how header include paths are specified.

> - Support for methods, namespaces, overloaded functions.

> 

>   Did you find the feature useful despite those limitations? I am interested to improve the situation in the future, unfortunately I find very little time to work on this area recently, but I do welcome every changes.


Yes I think this is useful despite the missing features. With the current model file design I am confident those features could be easily added at a later stage?

> I have two comments before I start to review the patch itself. Right now this patch contains two modifications. One for the .model files and one for a checker. I think it would be better to separate these two changes into two separate patches. If the motivation behind the merge of those patches is that, you want to test a feature you implemented in .model files, than I think there are other checker that are using attributes, so I think you should be able to provide a test case with the two changes separated. (For example using the nonnull parameter checker.)


I see, yes using the nonnull param checker for testing seems a better fit, I will update the patch.

> The other comment is that, the .model files are intended to work in a way, that checkers should not be able utilize the information whether some data is coming from a model file or the analyzed translation unit. In fact, this is an abstraction, that makes it possible to develop the checkers and the modelling mechanism independently. With you patch, the checker should observe the model declaration explicitly. I think, a better design would somehow merge the attributes that are coming from the original translation unit with the attributes coming from the model files, and expore that set of attributes. This way the checker could not tell what the source of the information is. Unfortunately I do not know what would be the best way to enfore this, since the checkers can just observe the AST of the original translation unit and bypassing the models.


I agree, abstracting the attributes' origin is nicer, hopefully we can find a solution that is elegant enough. I will have a look.
Thanks!

Pierre


http://reviews.llvm.org/D13731