cpp plugin: visitor & context madness

[David Nolden]

Am 25. Oktober 2011 21:32 schrieb Milian Wolff <mail at milianw.de>:
> Hey all,
>
> I want to cleanup the cpp plugin a bit after I was hit by the bug fixed by:
> http://commits.kde.org/kdevelop/5a68522b2197da7442ca34bd31a89dd7db013d66
>
> while at it I found a few strange things I'd like to have some feedback on:
>
> a) NameASTVisitor & TypeASTVisitor have m_context + m_localContext which are
> both set via the ctor and never changed thereafter. the only use I could find
> is ExpressionVisitor::visitSignalSlotExpression - could someone
> explain/comment on this?

But they are initialized with different values. The first is the
context to search for the name in, and the "localVisibilityContext" is
the context where "unbound" expressions are evaluated. Look at
ExpressionVisitor::findName.

Example: "typedef int X; Bla bla; bla.member<X>();"

While searching "member", "context" will be the class-context of
"Bla", and localContext will be the context where where "member" is
called, and from with "X" will be resolved (otherwise it would not be
found). See NameASTVisitor::processTemplateArgument for that.

> b) neither NameASTVisitor nor TypeASTVisitor track the "current" context, i.e.
> node->ducontext and just pass along what they found in their ctor - is this
> OK? should we not maybe also overload visit() there and introduce a
> currentContext (or could we reuse one of m_context/m_localContex)?

It is expected that these are called only on ASTs which don't
introduce new contexts (we're just looking up a type or a name),
therefore it won't matter.

> c) the expression visitor has two TopDUContext* members: m_source and and
> m_topContext. the former gets set in the ctor, the latter in parse(). why are
> two required, what is the difference? could someone comment/explain?

m_source is a top-context which may be "higher" in the #include chain,
and therefore has more visibility, and from which the "m_topContext"
we are currently pasing is included. This is often required in C++,
because headers often don't include everything they need, but rather
must be included in the correct order to work.

> d) the ExpressionParser takes a raw TopDUContext* (default = 0) argument but a
> DUContextPointer for the context argument. the latter and the code in
> evaluateType make it clear that this can be used later on, e.g. in code
> completion. so
>
> - shouldn't the topcontext argument also be a smart pointer?
> - we set the ast->ducontext to context.data() and do one more check for
> validity, but down below the ExpressionVisitor will be used which never again
> does any checks. isn't this *very* unsafe?

No, because there may be only one parse-thread at a time working on a
specific context. If we are the parse-thread, then we can expect
nobody else to do changes besides us. Otherwise the whole parser code
would be much more inefficient, as the whole smart-pointering would
have a high price when done on such a fine-grained level.

> the API dox of the expression parser says:
>
>> The duchain does not strictly need to be locked when this is called, but it
>> should be locked if the entity evaluating the expression has no control over
>> the lifetime of @p context
>
> can we somehow make this clearer on an API-level - ideas anyone?

It basically means what I have said above: The parse-thread is the
only possible changer of the context, so it can be sure that the
object isn't changed, and therefore it can call the expression parser
without holding a duchain lock. When another thread uses the
expression parser, then the duchain must be locked over the whole
time, since there are no fine-grained checks in the expression parser.

Maybe the comment could be changed to "The duchain must be locked when
this is called, unless it is called from within the background
parser.".

> e) the visitor and some other api uses "const DUContext*", others expect just
> a DUContext*. Theroetically I prefer the former, but as soon as you try to use
> DUContextPointer / IndexedDUContext or similar contains, you must use a
> const_cast - or am I missing something? Could this be improved? Should we use
> plain non-const pointers everywhere?

I'm not sure. The whole "const" and "non-const" doesn't really makes
sense unless it is synchronized with the duchain-locking, which is the
_real_ thing deciding whether you can change or not. I guess it would
be best to simply change everything to non-const.