This is an archive of the discontinued LLVM Phabricator instance.

[ASTImporter] Changed use of Import to Import_New in ASTImporter.
ClosedPublic

Authored by balazske on Oct 29 2018, 8:46 AM.

Diff Detail

Event Timeline

balazske created this revision.Oct 29 2018, 8:46 AM

The Import functions are replaced by Import_New and error handling is added. This is done in the implementation of the Import_New functions itself, and in importInto. (Code of old Import functions is moved to the Import_New and updated to use Import_New.) Some other functions in ASTImporter are still using the Import, these are to be changed later.

balazske updated this revision to Diff 171519.Oct 29 2018, 9:22 AM

Reformatted code.

Hi Balasz,

As I guess, the next step is to convert all Importcalls to Import_New and then rename Import_New into Import. If so, why aren't we able to just change the behaviour of Import methods?
Also, there are some comments inline (review is not fully completed, I'll continue review after we agree if this patch is needed).

include/clang/AST/ASTImporter.h
192–198

an import error?

198

these versions

lib/AST/ASTImporter.cpp
7917

We can use Import_New if we change this code, like it is done below.

7959

We can use -> here. Same in some cases below.

7960

If the true branch is enclosed with braces, the else branch should be enclosed as well.

7968–7969

This line exceeds 80-char limit.

balazske marked 3 inline comments as done.Nov 15 2018, 12:48 AM

If we change signature of Import now, other parts of the code (in clang and LLDB) would not compile (without changing to use the new kind of Import). If there is a Import_New the old code can still compile and can be changed later.

lib/AST/ASTImporter.cpp
7917

For IdentifierInfo there is no Import_New because this is the one kind of import that can not fail, it remains the same Import as before (does not return Expected).

balazske updated this revision to Diff 174168.Nov 15 2018, 1:13 AM
  • Small style corrections.
balazske added inline comments.Nov 15 2018, 1:21 AM
include/clang/AST/ASTImporter.h
192–198

I do not know what is the correct, but a specific import error is returned, that occurred during the import.

198

If the other comment is changed I can change this too, but alone this is not worth it. This is anyway a temporary comment, and if the first Import there is removed the second can not compile anyway (and the Import_New will be the Import later).

a_sidorin accepted this revision.Nov 25 2018, 8:37 AM

LGTM. Thank you for addressing my questions!

This revision is now accepted and ready to land.Nov 25 2018, 8:37 AM

LGTM. Thank you for addressing my questions!

Hi Alexei,

Could you please also take a look on that patch which this one depends on? https://reviews.llvm.org/D53751
I think the changes of that patch are included in this one too. Thank you.

balazske updated this revision to Diff 175649.Nov 28 2018, 3:17 AM

Rebase to newest master.

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

I reverted this change because it breaks a bunch of lldb tests (on MacOS, and probably on other platforms too).
To be clear, part of the reason I'm reacting strongly here is that this is not the first patch this has come up on.
I'm worried about the broader trend of landing patches to ASTImporter without proper reviews and testing as I am of this very instance.
I really think you should consider asking reviews to somebody who's familiar with clang and test lldb as it's the main client of this functionality we have in tree.

I reverted this change because it breaks a bunch of lldb tests (on MacOS, and probably on other platforms too). To be clear, part of the reason I'm reacting strongly here is that this is not the first patch this has come up on. I'm worried about the broader trend of landing patches to ASTImporter without proper reviews and testing as I am of this very instance. I really think you should consider asking reviews to somebody who's familiar with clang and test lldb as it's the main client of this functionality we have in tree.

@davide I am terribly sorry about this issue. I just applied this patch on my Linux box, and it did not failed those tests which failed on the green lab buildbots. Could you please refer to the Linux buildbots which failed? Seems like there is a significant difference between the testsuites on Linux and MacOS. Our primary target is Linux and we do run the LLDB tests before we accept any commit into our fork and only then we continue with upstreaming to Phabricator.

Our platform in our CI is Linux only, but we are in the middle of getting a hosted MacOS to support our CI. I do hope that in the near future there will be no such issues. In the meantime, is there a way for us to execute a green lab MacOS build bot with a specific patch before a commit? If that is not possible, then we can just promise that we will monitor your buildbots and we will do the revert.

I reverted this change because it breaks a bunch of lldb tests (on MacOS, and probably on other platforms too). To be clear, part of the reason I'm reacting strongly here is that this is not the first patch this has come up on. I'm worried about the broader trend of landing patches to ASTImporter without proper reviews and testing as I am of this very instance. I really think you should consider asking reviews to somebody who's familiar with clang and test lldb as it's the main client of this functionality we have in tree.

@davide I am terribly sorry about this issue. I just applied this patch on my Linux box, and it did not failed those tests which failed on the green lab buildbots. Could you please refer to the Linux buildbots which failed? Seems like there is a significant difference between the testsuites on Linux and MacOS. Our primary target is Linux and we do run the LLDB tests before we accept any commit into our fork and only then we continue with upstreaming to Phabricator.

I have no Linux bots access, this fails on macos (looks on http://lab.llvm.org:8080/green/). Although now it's too late because the logs are gone.

Our platform in our CI is Linux only, but we are in the middle of getting a hosted MacOS to support our CI. I do hope that in the near future there will be no such issues. In the meantime, is there a way for us to execute a green lab MacOS build bot with a specific patch before a commit? If that is not possible, then we can just promise that we will monitor your buildbots and we will do the revert.

I'm not aware of a way of pre-commit testing this hosted on llvm.org. I generally run on my machine and check the bots output.

balazske reopened this revision.Nov 29 2018, 6:44 AM

Reopening to fix failing lldb tests.

This revision is now accepted and ready to land.Nov 29 2018, 6:44 AM
balazske updated this revision to Diff 182866.Jan 22 2019, 2:04 AM
  • Small style corrections.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 5 2019, 12:07 AM
balazske updated this revision to Diff 189696.Mar 7 2019, 5:15 AM
  • Small style corrections.
This revision was automatically updated to reflect the committed changes.