Page MenuHomePhabricator

[clang][ASTContext] Don't load external comments from Sema::ActOnDocumentableDecls
AbandonedPublic

Authored by jkorous on Apr 9 2019, 6:44 PM.

Details

Summary

Loading of external comments is expensive for large AST files and not necessary for all use-cases.

The way I understand the intention in ActOnDocumentableDecls is that we are just checking if some of existing comments should be attached to newly created Decls. I assume we don't need to load external comments since those Decls are "local".

SemaDecl.cpp:12390

// There is at least one comment that not attached to a decl.
// Maybe it should be attached to one of these decls?

Diff Detail

Event Timeline

jkorous created this revision.Apr 9 2019, 6:44 PM

The way I understand the intention in ActOnDocumentableDecls is that we are just checking if some of existing comments should be attached to newly created Decls. I assume we don't need to load external comments since those Decls are "local".

The point of ActOnDocumentableDecls is not to just check if comments should be attached. That can be done later, lazily. The point is to emit warnings in doc comments, which requires parsing the comment.

I feel like this change is making the APIs significantly worse, especially the high-level getCommentForDecl. This change is exposing an obscure low-level knob that many users don't know what to do with.

I think the root of the performance problem is that ActOnDocumentableDecls calls getCommentForDecl at all. Yes, it is convenient, however, it performs a lot of extra work -- like searching for comments on related decls, searching for comments on redecls etc., when all we want is to check whether these specific decls have any comments attached directly to them, and if yes, parse the comments. I think it would be better if you could add a function that does exactly that, and call it from ActOnDocumentableDecls.

I agree and will try to decrease the impact on interfaces.

On the functional side - does this look like a reasonable idea?

On the functional side - does this look like a reasonable idea?

Yes, totally.