Community
Participate
Working Groups
The PDOM currently doesn't store: - visibility: public, protected, private - storage class: auto, static, extern, mutable, register - other annotations inline, const, volatile - whether a function accepts varargs A side effect of this can be seen during content assist, where all methods of Class1 appear public: class Class1 { void f(); public: void g(); }; int main() { Class1 c; c.<invoke content assist here> Since most of these annotations are boolean flags (except visibility), we could store all these annotations as a bit vector in the database. 2 bits would be enough for visibility, and 9 bits for the other flags, leaving 5 bits for future use. Thoughts?
The private methods showing up in content assist are unrelated to the contents of the PDOM. Neither the IASTName.resolvePrefix nor the DOMCompletionContributor are filtering the results based on visibility.
I think storing these properties makes a lot of sense. For implementing the type hierarchy, some of this will be necessary.
Jason is going to take a look at this if there are no objections.
To enable full support for these flags, I'd need to add the bit vector to the following classes: - PDOMCField - PDOMCFunction - PDOMCPPField - PDOMCPPFunction - PDOMCPPMethod - PDOMCPPVariable - PDOMCVariable - PDOMCPPParameter - PDOMPointerType - PDOMQualifierType These classes would share code for storing/retrieving the individual flags. Their common ancestor is PDOMBinding, but it doesn't seem like a natural place for putting the new code (or is it?). Another alternative would be to have a utility class with static methods for packing/unpacking the bit fields; but this approach isn't very object oriented. Does anyone have any preferences about where the new code should live? Right now, I'm tempted to put it in PDOMBinding.
Be careful not to make the PDOM bigger than it has to be. It's already too big. Put new information where it needs to be. If that means duplicating code, then do so while cursing Java's lack of multiple inheritance. I'm getting quite good at it myself. :)
Thanks for the feedback. I'm trying as much as possible to minimize the space impact these changes will have. I analyzed which flags/modifiers apply to which DOM nodes. isMutable, isDestructor, and isVirtual and visibility (2 bits) only apply to C++ nodes (5 bits total), while the other flags apply to both C and C++ (8 bits total). So the record sizes of these nodes will grow by 1 byte: PDOMPointerType PDOMQualifierType PDOMCField PDOMCVariable PDOMCFunction ... and the sizes of these will grow by 2 bytes: PDOMCPPVariable PDOMCPPParameter PDOMCPPField PDOMCPPFunction PDOMCPPMethod Since space is allocated in the PDOM 16 bytes at a time (if I remember correctly), these changes will only increase the space usage of PDOMCPPFunction and PDOMCPPMethod. These nodes currently sit at 16 bytes per record, and they'll increase to 18 bytes (which means 32 bytes per record on disk). All the other affected nodes are below the 16 byte border.
Doug, I'm seeing what you mean now about multiple inheritance. Oh, how I loathe myself for succumbing to Ctrl-V :)
Hmm... is there a particular reason why PDOMCPPParameter extends PDOMNamedNode instead of PDOMCPPBinding? I'd prefer to have it extend PDOMCPPBinding so I could reuse some code. Any objections?
I think I see why the hierarchy is like that. PDOMBinding stores declarations, definitions, and references. Storing that for PDOMCPPParameters too would really bloat the PDOM. Hmm...
Created attachment 51801 [details] Proposed patch This patch adds CV-qualifiers, storage class specifiers (where allowed by the ISO C/C++ specs) and function/method annotation (takesVarArgs, isDestructor, isVirtual, isInline, isMutable) to the PDOM.
looks good to me :) one thing though - the overloading tests are giving false positives as they are testing for a single binding for a group of overloaded function/methods. We should have a binding per entity. just so you are aware - I've got a fix for the this (156504), and some related unit tests (which don't overlap significantly), but will hold off until this patch is committed as I'll move to using the convenience methods you've added to PDOMTestBase https://bugs.eclipse.org/bugs/show_bug.cgi?id=156504 also, I'm still seeing multiple non-deterministic failures due to the remaining race condition (this only happens without the suggested patch). I'm wondering why no one else is seeing these - I'm running under Win2k, so this might be the difference https://bugs.eclipse.org/bugs/show_bug.cgi?id=157992
I'm going to commit this. Question: should we do this for just 4.0 or the 3.1.x branch as well? I think we're getting borderline with respect to what is a fix versus an enhancement with all of this indexer work.
I have to rework the patch to incorporate Markus' new API changes. I'll have a new one ready to go Very Soon(tm) :)
I'll pull out the misleading overload tests too. The references/declarations/definitions shouldn't be counted the way I was counting them.
Yeah, just do this in HEAD. This is a pretty significant change and I don't think CDT 3.1.2 needs that kind of churn.
Created attachment 51873 [details] Updated patch The (operator/function) overloading tests I'm withholding account for the 10k difference between this patch and the previous one.
Chris, although this is assigned to you, I have commited the patch because I have another one that makes changes to the PDOM tests. I hope that's ok for you. Jason, I have integrated the failing tests with the regular ones. The advantage of that is, that you only have to remove the leading underscore to make a regular test case out of them. Please have a look whether this works for you.
Markus: Thanks very much. I originally had the failing test cases set to "fail" when the behaviour they test is fixed. That way, we'd get a reminder to update the test cases. Anyway, it's fine the way it is now. We'll just have to remember to remove the underscores once type information gets stored in the PDOM.
(In reply to comment #17) > Chris, although this is assigned to you, I have commited the patch because I > have another one that makes changes to the PDOM tests. I hope that's ok for > you. No problem. I'll mark this as fixed.
(In reply to comment #18) > Markus: Thanks very much. I originally had the failing test cases set to > "fail" when the behaviour they test is fixed. That way, we'd get a reminder > to update the test cases. That is still the case, the tests with the leading underscore are run and checked for failure. The method BasicTestCase.suite(Class clazz) does the magic.
