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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
