In D115374#3181670, @logan-5 wrote:

To be clear, it sounds like we should either add .take() for moving the string out of raw_string_ostream's string reference, or make raw_string_ostream not need to be flushed (after which there won't be as clear a use for .take(), since you can just use the underlying string directly).

I vote for the second option, making raw_string_ostream not need to be flushed, since it allows for simpler code at the call site (return Str;), permits NRVO at the call site, and avoids some possibly weird questions .take() would bring with it (like whether it would ever be surprising that it moves out of a reference that someone else might also have).

If that's the direction that sounds best, I can submit an updated patch sometime tomorrow.

LGTM. Sorry for missing this before. Looks to me like the debian build failure above is unrelated.

LGTM; sorry for missing this previously! (I'd have been happy for you to commit this without review, BTW, since it seems a bit obvious, but no excuse of course for me being slow...)

In D115374#3181491, @logan-5 wrote:

In D115374#3181383, @dexonsmith wrote:

I don't feel strongly, but IMO the code might be a bit harder to read/maintain with the explicit flush. I worry that it'd be easy to move the flush() away from the return. Not sure I'm right; could just be familiarity with str().

I definitely hear you. I don't really mind it personally, and I did it this way because I saw precedent in a couple spots (there's one on CompilerInvocation.cpp:653, several in clangd, etc.). I definitely see how it could be a little bit spooky though.

This patch fixes an issue in which SSA value reference within a DIArgList would be unnecessarily dropped by llvm-link, even when invoking on a single file (which should be a no-op).

Adding a couple of other possible reviewers.

Adding a couple of potential reviewers.

Adding some reviewers that have touched llvm/lib/Support/Windows recently. Note that I don't have access to a Windows machine and CI hasn't returned so I cannot confirm yet that this builds.

In D115391#3180980, @dblaikie wrote:

Probably worth committing separately in case either causes breakage.

I don't feel strongly, but IMO the code might be a bit harder to read/maintain with the explicit flush. I worry that it'd be easy to move the flush() away from the return. Not sure I'm right; could just be familiarity with str().

LGTM

This looks really nice.

In D115380#3180943, @dblaikie wrote:

Sounds good - if the only places we're trying to use unitiliazide values is where they're POD (can you confirm that's the case/maybe mention it in the commit message that the extra power was unneeded), then removing this more powerful version that could be used for non-POD uninitialized values seems helpful.

Note: I've posted follow-up patches that use this:

• https://reviews.llvm.org/D115384
• https://reviews.llvm.org/D115386
• https://reviews.llvm.org/D115389
• https://reviews.llvm.org/D115390
• https://reviews.llvm.org/D115391

Note: I've posted follow-up patches that use this:

• https://reviews.llvm.org/D115384
• https://reviews.llvm.org/D115386
• https://reviews.llvm.org/D115389
• https://reviews.llvm.org/D115390
• https://reviews.llvm.org/D115391

Re-uploading diff to trigger bots now that dependent patches are live.

LGTM.

LGTM, except I'd prefer the #includes be changed separately since that cleanup seems unrelated to this change.

In D115254#3176564, @jamesfarrell wrote:

Only change from previous attempt is to call rtrim() on the output of the commands in the unit tests, since the new version parsing code fails if there are leftover characters like \n.

LGTM.

In D114968#3167519, @jansvoboda11 wrote:

Assuming the filesystem doesn't change during dependency scanning, this change keeps the consistency between stat/read calls for minimized files.

Thinking about it some more though, the original reason for reading files eagerly (even for stat calls) was most likely to avoid inconsistencies when the file changes on the filesystem between stat and read. Is that correct, @arphaman? If so, I think I should abandon this patch.

Thanks for working on this; seems like a great start. At a high-level:

• We should check overhead. It'd be good to benchmark scanning LLVM with clang-scan-deps before and after this change.
• The locking is getting harder to track, since the acquisition and release are disconnected. I'd rather use a pattern that kept this simple.
• Identified a few pre-existing issues that might be exacerbated by (and/or harder to fix after) this refactor.

In D113812#3132357, @Meinersbur wrote:

In D113812#3129072, @dexonsmith wrote:

Huh... when did metadata get added to function declarations?

Since D21052

In D114860#3165439, @CodaFi wrote:

I think deleting functions with proper alternatives is fine, probably one at a time though so people can work through updating calls to those functions.

This is not, and has never been, the policy for deprecation for C API functions. We have a strict policy of ABI compatibility (for better or for worse) in these C bindings, these functions must stay.

This LGTM by the way (not sure if that was already clear, since you abandoned a different review than I anticipated when squashing, and I'd "accepted" the other one); also see my suggestion to move the call to getFileInfo inside of markIncluded (might be error prone not to make that change).

In D109459#3043589, @ldionne wrote:

@dexonsmith has had useful views on ABI changes in the past, I'm curious to have his opinion on this change, since I'm still ambivalent.

@arphaman , can you take a look?

SGTM; I'll let @jansvoboda11 confirm though.

In D104827#3137807, @scott.linder wrote:

Is this moving in the right direction? Does the restriction on DIArgList seem
reasonable, and if so does it need more explicit documentation or more thorough
enforcement?

I pushed this before looking at the intervening comments, so the last bit is a bit redundant; it sounds like the restriction on DIArgList was intended and makes sense, so I think the remaining question is whether the Verifier check is sufficient.

LGTM.

I have a couple of nits inline, but otherwise LGTM if you squash with https://reviews.llvm.org/D114095.

I'm not sure this commit stands on its own, since it introduces a (temporary) regression in .pcm size; I suggest squashing with the follow-up, https://reviews.llvm.org/D114096.

LGTM with one nit inline.

Thanks for the reviews! Pushed in fd6018072ace7d5cdf537fd63a44c050a98e52fc.

Oh, a couple of comments on alternatives:

• I considered moving the mutable keyword from iterator::Operation to Operation::Error, just limiting its scope. But since the change is completely ignored and it's reasonable to have a const-qualified verify() function this seems simpler.
• Another approach would be to make Operation::verify() a static local function private to DWARFExpression.cpp. It accesses private fields, but it'd be easy enough to rewrite it to use accessors.

I pushed fa39ec5786c08e78cc247a48c17aef66ca5985fb, a35226b9b606f4ea32e7db9e2f4de0d99ab6bef0, and 7a1b670f0f2f0a38a673b5554bb2c4d9d50447bf to fix most of the blockers. Then https://reviews.llvm.org/D113957 fixes Operation::verify() and https://reviews.llvm.org/D113958 makes it iterate over const Operation and removes the mutable.

[No change; just re-triggering builds after telling Phabricator this depends on the other patch.]

LGTM.

In D109128#3097060, @keith wrote:

@dexonsmith turns out the test I was adding is broken on main today too. If it's alright with you I will punt that to another diff to not increase the scope of this one.

Huh... when did metadata get added to function declarations? I'm not sure what the semantics of those are supposed to be. I seem to remember that been left out when metadata was added for definitions.

Thanks for the reviews! Pushed in 6b9b86db9dd974585a5c71cf2e5231d1e3385f7c.

@mehdi_amini @rriddle @silvas, FYI, my git-grep turned up a few of these duplicated paramaters in MLIR; not sure if you're interested in cleaning them up (I don't have a build going for it myself). The ones I happened upon were in:

• Region::OpIterator
• ValueUseIterator
• ValueUserIterator
• ResultRange::UseIterator