@martong the only issue is that I am seeing a regression on Analysis/ctu-main.cpp when I run check-clang. I am going to look into it but if you have any insights that would be helpful.

Thank you, this looks good but perhaps some refactoring would help improve the change.

Sorry for the late review

Can you add a test?

Thank you for adding the additional test.

Sounds good!

LGTM after comment are addressed.

I find the static_cast<bool> to be a bit too clever, I don't think it helps readability.

See changes here

The fix was not too bad, I landed the fix and the bots just turned green http://lab.llvm.org:8080/green/view/LLDB/job/lldb-cmake/14015/

Just a heads up this change broke the lldb std::function formatter see the logs here:

This PR broke the green dragon bots, see the following log file:

As pointed out in this comment in another review

Apologies, I meant to make the comment in the child PR

As pointed out in this comment in another review

In D53751#1311037, @martong wrote:

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.

Apologies for my late comments.

The rest of the changes look good.

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.

Did you ever resolve the issue of libcxx tests not running https://reviews.llvm.org/D53697

LGTM, thank you for fixing this.

@zturner I would not be against discussing using pass by value for small objects going forward. I don't currently have a good feeling for at what sizes/data types the right trade-off is at though.

@jingham @zturner @teemperor @clayborg I believe I have addressed all the comments.

Made changes based on comments.

@zturner I was out of date when I posted this diff but I have that file updated locally and it will show up when I update the diff.

@zturner I don't see AddEnumerationValueToEnumerationType being called in SymbolFile/NativePDB.cpp https://github.com/llvm-mirror/lldb/search?q=AddEnumerationValueToEnumerationType&unscoped_q=AddEnumerationValueToEnumerationType

Much better, thank you!

@martong Hello, if I look at the specific test that was failing TestDataFormatterLibcxxVector.py it only has the following decorator @add_test_categories(["libc++"]) so that should not prevent it from running on Linux if you are also building libc++. If you do a local cmake build and you build libc++ as well does it still skip the test?

I reverted this commit since it caused a regression in the lldb test suite, specifically TestDataFormatterLibcxxVector.py.

I am happy to discuss and review a revised version of this change.

Are we refactoring in the right place? Why not just refactor FileSpec::GetByteSize() why is FileSpec the wrong place?

LGTM

+1 for modernizing code!

@stella.stamenova Thank you for catching this. I fixed the test names, I am looking into the best way to fix the skipif now.

@jingham removed code and made it NO_DEBUG_INFO_TESTCASE

Removing uneeded code and making test NO_DEBUG_INFO_TESTCASE

@jingham I believe I addressed your comments, please review again.

• Addressing comments
• Adding test to make sure we step through a std::function wrapping a member variable

Addressing comments and fixing a bug in ThreadPlanStepThrough.cpp where I would overwrite the result of objc_runtime.

First batch of comments, may come back to it today.

Adjusting documentation based on feedback

@jingham @clayborg @davide addressed comments w/ the exception of the lambda one which I politely disagree with.

Addressing comments:

Updating LibcxxVariantGetIndexValidity() to no longer do type check of __index. It was left over from the old method of checking for an empty variant and was also breaking clang 5.

@teemperor Thanks for the catch!

Applying clang-format to files I missed earlier

Applying clang-format

@vsk Interesting I ran git clang-format before generating the diff and it made changes, so I am not sure what happened

@vsk @jingham I believe I have addressed your comments, please review again.

Address comments on std::variant formatter

• Adding tests for reference to variant and variant of variant
• Refactoring test to be more efficient
• Refactoring unsafe code that assumed sizes of types between local machine and target machine were the same
  • Used to check to variant_npos which is max unsigned or -1