[libcxx-commits] [PATCH] D112374: [clang] Implement ElaboratedType sugaring for types written bare

Sat Jul 16 11:16:05 PDT 2022

mizvekov reopened this revision.
mizvekov added a comment.

In D112374#3653967 <https://reviews.llvm.org/D112374#3653967>, @JDevlieghere wrote:

> I'm sorry to hear you're having trouble building LLDB. The LLDB website has quite an elaborate guide with instructions in how to build LLDB: https://lldb.llvm.org/resources/build.html, including specific instructions on Windows.

The instructions exist, doesn't mean they work or that they are a fair burden on the developers of the other projects.

The LLDB build / test system is made of the same 'stuff' as the rest of LLVM sure, but it does a lot more 'questionable' things which makes it look more hazardous.
For example, opening hundreds of frozen terminal windows or creating paths in the build directory which contain unsubstituted variables.
So it seems to me that for me to be comfortable working with this, I would have to adjust my workflow to build LLVM in a sandbox instead.

If we tried polling other clang devs, we might find that standard practice is that we are not really even trying to build and run LLDB tests locally anymore.

libcxx devs have a CI pre-commit system which only runs when a path in their sub-project is touched. I think this would be reasonable start for LLDB.

Lastly, my main concern is that by keeping this patch off, even if we don't suspect a problem in it, this will create a very long tail. The users affected don't know about it yet, and they will keep coming
with a delay one by one as we re-land and revert.

> I'm happy to help out. I personally don't know if we should go with (1) or (2), both sound reasonable in their own way. I'd like @teemperor, who's the author of the feature and the affected tests, to weigh in.

I think, but can't confirm, that this is just a case of (1) and for what is being tested we don't really care how the return type of those size() methods is written.
I would like some way to test that the functionality is not really broken, we just changed that test expectation, but alas I can't.

> I don't. I think reverting your change was well within the guidelines outlined by LLVM's patch reversion policy: https://llvm.org/docs/DeveloperPolicy.html#patch-reversion-policy
>
> Additionally, I think you could've given me a little bit more time to catch up on the discussion here. The code review policy and practices (https://llvm.org/docs/CodeReview.html#code-reviews-speed-and-reciprocity) recommend pinging every few days to once per week depending on how urgent the patch is.

I think, given the size of this patch and the other points I made, we could have simply fixed those issues post-commit, if I had received any prior notification.

In D112374#3657332 <https://reviews.llvm.org/D112374#3657332>, @kimgr wrote:

> It's a little confusing, because it now looks like _every_ `Type` in the AST is wrapped in an `ElaboratedTypeLoc` + `ElaboratedType`. IWYU's debug AST dump shows this (excerpt):
> I'm not sure I understand why the elaborated type nodes are necessary even when they don't seem to add any additional information?

It's the difference in knowing the type was written without any tag or nested-name specifier, and having a type that you are not sure how it was written.

When we are dealing with a type which we are not sure, we would like to print it fully qualified, with a synthetic nested name specifier computed from it's DC, because otherwise it could be confusing as the type could come from somewhere very distant from the context we are printing the type from. We would not want to assume that a type which has been desugared was written how it's desugared state would seem to imply.

FWIW, in the state of affairs we leave clang after this patch, I don't think it's worth keeping a separate ElaboratedType anymore, we might as well fuse it's functionality into the type nodes which could be wrapped in it. Taking care to optimize storage when not used otherwise, I think we can recoup the performance lost in this patch, perhaps even end in a better state overall.

But I think doing these two steps in one go would not be sensibly incremental. We have in this patch here a very simple core change, which is very unlikely to have bugs in itself, but creates enormous test churn.

The second step of eliminating ElaboratedType could be a less simple core change with almost zero test churn, which makes it less risky that it would introduce a bug that escapes review.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D112374/new/

https://reviews.llvm.org/D112374