Thanks for the checker! We are hitting a crash on _Atomic fields, could you take a look? https://bugs.llvm.org/show_bug.cgi?id=41590 Thanks!

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

I'd be happy to land it, but I do want @rsmith to take a look.

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".

This fix makes sense to me, however, Richard Smith @rsmith is the best person to review this patch.

Do you have commit access?

Submitting code as it becomes ready is the usual practice here.

As far as I'm concerned, I don't have any major revision requests. I haven't reviewed the unit tests (I'm planning to), but for non-test code, I don't have any further comments.

In D59130#1426579, @krytarowski wrote:

I would port it to NetBSD... but I don't see use-case for it. I think kernel should be allowed to schedule priorities on its own without manual help.

In D59130#1426403, @jfb wrote:

In D59130#1426086, @gribozavr wrote:

In D59130#1424778, @jfb wrote:

That doesn't answer my question though: if someone sets priority and on the platform the test witness acceptable forward progress, will it happen that another platform provide no forward progress for the same work. For example, some implementations don't let a background thread perform any I/O. In such a case, a mere logging statement would halt the background thread's progress.

Fair enough. But is this a theoretical concern for some abstract platform that might implement this function in future, or is it a concrete concern for one of the implementations in this review?

You're adding a "portable" abstraction to different platform APIs. The onus here is on you to make sure that the abstraction is portable

Sorry for the delay. I will finish reviewing tomorrow.

I basically didn't really like the idea of testing against hard-coded hash values in unittests as I consider it to be an implementation detail.

I left some comments, but it is difficult for me to review without understanding what the requirements for this hasher are, why some information is hashed in, and some is left out, could you clarify? See detailed comments.

In D59130#1424778, @jfb wrote:

That doesn't answer my question though: if someone sets priority and on the platform the test witness acceptable forward progress, will it happen that another platform provide no forward progress for the same work. For example, some implementations don't let a background thread perform any I/O. In such a case, a mere logging statement would halt the background thread's progress.

Fancy!

Do we also need to change anything for constructor calls? If not, please add a test for that.

My understanding is that the storage space taken up for Refs is currently 8 bytes per Ref (4 each for the start and end positions), plus filename strings which are deduplicated across all refs. If we add a SymbolID, that adds an additional 8 bytes to each Ref. Given that Refs are numerous, and most of them won't use the SymbolID, that seems wasteful.

Hmm, do we really need the matches for the AST node that is not described/defined by the standard? If this is really so, I'm ok with this.

The test coverage i have will be in the clang-tidy check.

These references were added to support using the index data for symbol rename.

Please also add a test for find references on a constructor.