Page MenuHomePhabricator

[ASTImporter] Added Import functions for transition to new API.
ClosedPublic

Authored by balazske on Oct 26 2018, 2:27 AM.

Details

Summary

These Import_New functions should be used in the ASTImporter,
and the old Import functions should not be used. Later the
Import_New should be renamed to Import again and the old Import
functions must be removed. But this can happen only after LLDB
was updated to use the new Import interface.

This commit is only about introducing the new Import_New
functions. These are not implemented now, only calling the old
Import ones.

Diff Detail

Repository
rL LLVM

Event Timeline

balazske created this revision.Oct 26 2018, 2:27 AM
balazske updated this revision to Diff 171278.Oct 26 2018, 4:00 AM
  • Corrected Import_New(const Attr *)
balazske updated this revision to Diff 171292.Oct 26 2018, 6:22 AM
  • Added missing Import_New with Selector and DeclarationName.

I think these changes make sense at a high level but I am not sure about the refactoring strategy. I am especially concerned we may end up in place where all the effected users of the API don't get updated and we are stuck with this parallel API.

Tagging in @rsmith since he has reviewed a lot of recent changes involving ASTImpoter that I have seen recently and he will have a better feeling for how these types of refactoring on handled on the clang side. I am mostly focused on the lldb side but care about the ASTImporter since we depend on it.

I think these changes make sense at a high level but I am not sure about the refactoring strategy. I am especially concerned we may end up in place where all the effected users of the API don't get updated and we are stuck with this parallel API.
Tagging in @rsmith since he has reviewed a lot of recent changes involving ASTImpoter that I have seen recently and he will have a better feeling for how these types of refactoring on handled on the clang side. I am mostly focused on the lldb side but care about the ASTImporter since we depend on it.

Hi @shafik ,

I completely understand your concern. We modify ASTImporter in order to make cross translation unit (CTU) analysis working on C++ projects. During these modifications we carefully try to keep LLDB functionality intact.

This patch is one of the last patches of a refactor in ASTImporter about using Error and Expected<T> as return types. We need this in CTU analysis because

  • we want to distinguish between different kind of errors
  • internally in ASTImporter we'd like to enforce the checking whether there has been any error during the import of any subexpressions

After this patch our next patch would rename these Import_New functions to Import and the old Import function implementations returning with a raw pointer would be deleted. This indeed would effect LLDB, thus we are going to create an LLDB patch too. We already have that LLDB change in our fork (https://github.com/Ericsson/lldb/commit/7085d20) and soon we will put that in Phabricator too.

Now, my concern is that waiting for the approve from @rsmith could take several months since he is usually very overwhelmed. Unfortunately, we have several other changes which depend on this change, so this is a blocker for us. Also, I think that @a_sidorin has the greatest experience in ASTImporter code. Would it be okay for you to accept this patch once Alexei approved it?

Hi Balazs,
The changes look mostly fine to me. I think they can be accepted after some minor stylish fixes.

Hi Shafik,

I think these changes make sense at a high level but I am not sure about the refactoring strategy. I am especially concerned we may end up in place where all the effected users of the API don't get updated and we are stuck with this parallel API.

As I understand, these changes are done exactly in order to perform a seamless transition into a new error-checking API. This was discussed in both cfe-dev and lldb-dev: https://lists.llvm.org/pipermail/cfe-dev/2018-April/057771.html.
It looks like the patches for the final transition will be ready soon. @balazske, @martong Am I correct?

Tagging in @rsmith since he has reviewed a lot of recent changes involving ASTImpoter that I have seen recently and he will have a better feeling for how these types of refactoring on handled on the clang side. I am mostly focused on the lldb side but care about the ASTImporter since we depend on it.

Review from @rsmith is extremely appreciated; however, me and @spyffe have reviewed most ASTImporter-related patches in the last two years. ASTImporter itself doesn't have any use in clang frontend. It was an LLDB bridge only until CSA started to use it too.

include/clang/AST/ASTImporter.h
244 ↗(On Diff #171292)

Please split this line and the changed line below.

lib/AST/ASTImporter.cpp
7882 ↗(On Diff #171292)

Looks like this line needs to be split too.

8254 ↗(On Diff #171292)

... and this one too.

a_sidorin edited reviewers, added: a_sidorin; removed: a.sidorin.Nov 25 2018, 11:07 AM
balazske updated this revision to Diff 175207.Nov 26 2018, 1:37 AM
  • Split long lines.
balazske updated this revision to Diff 175212.Nov 26 2018, 2:20 AM
balazske marked an inline comment as done.
  • Split long lines (ASTImporter.cpp).
balazske marked 2 inline comments as done.Nov 26 2018, 2:22 AM
a_sidorin accepted this revision.Nov 26 2018, 1:00 PM

LGTM, thanks!

This revision is now accepted and ready to land.Nov 26 2018, 1:00 PM
balazske updated this revision to Diff 175529.Nov 27 2018, 10:35 AM
  • Corrected Import_New(const Attr *)
  • Added missing Import_New with Selector and DeclarationName.
  • Split long lines.
  • Split long lines (ASTImporter.cpp).

Rebase to newest master.

This revision was automatically updated to reflect the committed changes.
davide added a subscriber: davide.Nov 27 2018, 11:34 AM

I think these changes make sense at a high level but I am not sure about the refactoring strategy. I am especially concerned we may end up in place where all the effected users of the API don't get updated and we are stuck with this parallel API.

I didn't actually see this comment get addressed other than to say it won't be a problem in practice, which I'm not certain I agree with. Was there a reason why this got commit before finding out if the reviewer with the concern agrees with your rationale? FWIW, I share the concern that having parallel APIs for any length of time is a dangerous thing. Given that "almost ready to go" is not "ready to go" but there's not an imminent release, I don't understand the rush to commit this.

When is the renaming and removal of the old API expected take place? Days? Weeks? Months?

It is really "ugly" thing to have a Import_New and a Import for the same functionality. Without these a single large patch would have been needed for clang and another smaller one for LLDB that must be committed "at the same time". Now 1 dependent patch is under review. Then 1 patch is to do for clang and one for lldb (these can contain the removal of Import_New too). It mainly depends on the speed of the reviews how fast the update happens.

I didn't actually see this comment get addressed other than to say it won't be a problem in practice, which I'm not certain I agree with. Was there a reason why this got commit before finding out if the reviewer with the concern agrees with your rationale? FWIW, I share the concern that having parallel APIs for any length of time is a dangerous thing. Given that "almost ready to go" is not "ready to go" but there's not an imminent release, I don't understand the rush to commit this.

@aaron.ballman Thanks for your time and review on this.

I completely understand you concern and agree that having such parallel API even for a short time is not good. Please let me explain why we chose to do this still:
ASTImporter::Import functions are used externally by LLDB and CTU as clients. However, the functions are used internally too, inside ASTImporter and ASTNodeImporter. E.g. when we import an expression, then first we use the Import(QualType) function to import its type.
Our goal is twofold: (1) enforce ASTImporter and ASTNodeImporter implementation functions to always check the result of used import functions and (2) make sure that clients can have detailed error information, so e.g. in case of CTU we can handle unsupported constructs differently than ODR errors.
As @balazske mentioned we could have changed the API in one lockstep but the impact would have been too huge.

In the context of this patch, you can think of the newly introduced Import_New functions as the internal only functions. I was thinking about that we could make them private and ASTNodeImporter as a friend, that way we could hide them from clients and then the dual API would cease to exist.

I didn't actually see this comment get addressed other than to say it won't be a problem in practice, which I'm not certain I agree with. Was there a reason why this got commit before finding out if the reviewer with the concern agrees with your rationale? FWIW, I share the concern that having parallel APIs for any length of time is a dangerous thing. Given that "almost ready to go" is not "ready to go" but there's not an imminent release, I don't understand the rush to commit this.

@aaron.ballman Thanks for your time and review on this.

I completely understand you concern and agree that having such parallel API even for a short time is not good. Please let me explain why we chose to do this still:
ASTImporter::Import functions are used externally by LLDB and CTU as clients. However, the functions are used internally too, inside ASTImporter and ASTNodeImporter. E.g. when we import an expression, then first we use the Import(QualType) function to import its type.
Our goal is twofold: (1) enforce ASTImporter and ASTNodeImporter implementation functions to always check the result of used import functions and (2) make sure that clients can have detailed error information, so e.g. in case of CTU we can handle unsupported constructs differently than ODR errors.
As @balazske mentioned we could have changed the API in one lockstep but the impact would have been too huge.

I believe an alternate approach would have been to change one function at a time and do this change across. These would have been smaller changes and easier to review. This would have prevented the need for an intermediate name which causes churn in the commit history.

In the context of this patch, you can think of the newly introduced Import_New functions as the internal only functions. I was thinking about that we could make them private and ASTNodeImporter as a friend, that way we could hide them from clients and then the dual API would cease to exist.

I didn't actually see this comment get addressed other than to say it won't be a problem in practice, which I'm not certain I agree with. Was there a reason why this got commit before finding out if the reviewer with the concern agrees with your rationale? FWIW, I share the concern that having parallel APIs for any length of time is a dangerous thing. Given that "almost ready to go" is not "ready to go" but there's not an imminent release, I don't understand the rush to commit this.

@aaron.ballman Thanks for your time and review on this.

I completely understand you concern and agree that having such parallel API even for a short time is not good. Please let me explain why we chose to do this still:
ASTImporter::Import functions are used externally by LLDB and CTU as clients. However, the functions are used internally too, inside ASTImporter and ASTNodeImporter. E.g. when we import an expression, then first we use the Import(QualType) function to import its type.
Our goal is twofold: (1) enforce ASTImporter and ASTNodeImporter implementation functions to always check the result of used import functions and (2) make sure that clients can have detailed error information, so e.g. in case of CTU we can handle unsupported constructs differently than ODR errors.
As @balazske mentioned we could have changed the API in one lockstep but the impact would have been too huge.

In the context of this patch, you can think of the newly introduced Import_New functions as the internal only functions. I was thinking about that we could make them private and ASTNodeImporter as a friend, that way we could hide them from clients and then the dual API would cease to exist.

Thank you for the explanation -- I guess I would have preferred to go with the suggestion from @shafik to have done this one API at a time, rather than as an entire set of APIs. However, given that the code is already in, I don't think it would be worth the churn to revert and go that route.