and run the test case, the first added line prints 1 and the second one 0. Result is being mutated when doing the in-place evaluation.

Thank for looking at this bug, just a small comment on the test.

I think I need to do another sweep of this PR but mostly minor comments.

Are you sure it is failing on the line you indicated? After looking at the code, a little before that line we have:

Any update/further details here?

• Expanded test
• applied clang-format

You might want to try fun cases like operator< and operator()() from a lambda. They should work but might be worth throwing them in.

In D123319#3473283, @dblaikie wrote:

('scuse the delay)

Baseline: I'm still not really sure this is the right direction. Is there a sound argument for why this change is suitable for lambdas, but not for other types? I believe all the situations that can happen with other types can happen with lambdas (& the other way around) with sufficiently interestingly crafted inputs.

This mostly makes sense, the purpose of the || alternate_defn was not clear to me either.

-Replacing CHECK-NEXT with CHECK

Thank you, the names make a lot more sense to me but I am not sure why this was originally used either.

-Removed override RemoteAwarePlatformTest.cpp and added NOLINT

In D123340#3438391, @labath wrote:

The reason for the funny /*override*/ thingy in the mock classes is that it is impossible (in the gmock version that we use) to add the override keyword to the methods overridden by the MOCK macros. Then, having override keywords on hand-written methods triggered -Winconsistent-missing-override.

Or at least it used to -- have you checked that these changes don't introduce any new compiler warnings?

• Making test more robust
• Adding comments

Adding test case

I did some experiments where I did not restrict this to lambdas case and I could not come up with a test case where we emit debug info and do not have the deduced type.

Note, in C++20 this was made well-defined see godbolt the paper that did this was P0907R4 Signed Integers are Two’s Complement

• Rebased
• Applied clang-format
• Address Pavel's comment

I believe that NSIndexPathSyntheticFrontEnd::Update() also needs a fix in order to properly set the active member but I will do that as a separate fix since.

Do we have any tests for these?

In D121831#3390152, @aprantl wrote:

Did you check if we have other helper expressions with the same problem?

We sure do have a lot of CommandOptions classes out there.

Note, we have a few tests that cover LoadImage:

Fixing up diff

I think this PR broke green dragon incremental build: https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/42185/

Removing dead code.

Can you add a test to ASTImpoterTest.cpp that checks that we import the LambdaDependencyKind correctly?

Applying clang-format

Addressing comment

This is just prep work for another change then?

There can probably be some more refactoring wrt to the Value class which also does a lot of the same work but there are enough differences that I think any attempt at that should be left for a separate PR.

In D121161#3368117, @JDevlieghere wrote:

In D121161#3367806, @labath wrote:

The class has a constexpr constructor. I thought that would be enough to avoid runtime initialization. Is this being flagged by something?

It is getting flagged by -Wglobal-constructor but you're right, it's constexpr and therefore shouldn't. Might be a bug in clang. I'll try to repro with ToT.

This make sense to me at a first pass, please make sure you run check-lldb as well.

As a side note, I noticed that we don't prefix typedefs from cstdint with std:: e.g. size_t. These typedefs are not guaranteed to be in the global namespace.

Looks like it also broke lldb green dragon incremental as well: https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/41865/consoleFull#-27141295349ba4694-19c4-4d7e-bec5-911270d8a58c

Moved to using namespace lldb_private::dwarf everywhere since the consensus is that being verbose is preferred.

Updating diff based on comments

In D120836#3355167, @labath wrote:

I think it's reasonable to be able to refer to the dwarf constants from within the dwarf plugin via their base names alone. The implementation -- a top-level using namespace llvm::dwarf -- is not reasonable, but that's because the plugin is very old, and completely unnamespaced.

For the newer plugins, we've started putting them in a sub-namespace of lldb_private, which means they cannot be accidentally referenced from the core code, but they can easily reference (without qualification) symbols defined in the core libraries.

If we put the dwarf plugin into a (e.g.) lldb_private::dwarf namespace, then I think it would be ok to put a using namespace llvm::dwarf into that namespace, even in a header -- because those symbols would only be visible to symbols which are in that namespace.

In other words, I'd have the dwarf plugin do what the minidump plugin is doing (disclaimer: I wrote most of that plugin).

Anyway, I'm not married to that approach, but that's what I would do...

I used using namespace in a few functions but I am not committed to this approach. So I am happy to hear feedback on whether we want to just use fully qualified names everywhere instead or nail down a criteria as to when it is acceptable.

In D120718#3352450, @JDevlieghere wrote:

I think the better solution here is to get rid of the using namespace llvm; in the implementation file instead.

LGTM

Looks like this PR broke the green dragon bot, see: https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/41771/consoleFull#10309019849ba4694-19c4-4d7e-bec5-911270d8a58c

Looks like the scripted_crashlog_json.test is broken on green dragon: https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/41454/testReport/junit/lldb-shell/ScriptInterpreter_Python_Crashlog/scripted_crashlog_json_test/

Used shorter options for renaming based on feedback.

Do we have a test that covers this edge case?

Addressed comments on SmallVector size and fixed test.