Page MenuHomePhabricator

dexonsmith (Duncan P. N. Exon Smith)
User

Projects

User does not belong to any projects.

User Details

User Since
Mar 18 2014, 10:33 AM (403 w, 1 d)

Recent Activity

Today

dexonsmith added a comment to D115374: [NFC][clang] Return std::strings built from raw_string_ostreams more efficiently.

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.

Wed, Dec 8, 6:07 PM · Restricted Project
dexonsmith accepted D114505: [clang][unittests] Fix a clang unittest linking issue.

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

Wed, Dec 8, 5:49 PM · Restricted Project
dexonsmith accepted D113254: [clang] Fix a misadjusted path style comparison in a unittest.

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...)

Wed, Dec 8, 5:38 PM · Restricted Project
dexonsmith added a comment to D115374: [NFC][clang] Return std::strings built from raw_string_ostreams more efficiently.

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.

Wed, Dec 8, 5:36 PM · Restricted Project
dexonsmith added a comment to D114355: [DebugInfo] ValueMapper impl for DIArgList should respect RF_IgnoreMissingLocals.

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).

Wed, Dec 8, 5:22 PM · debug-info, Restricted Project
dexonsmith resigned from D115031: [AST] Print NTTP args as string-literals when possible.
Wed, Dec 8, 4:47 PM · Restricted Project
dexonsmith added inline comments to D115386: AST: Avoid using SmallVector::set_size() in UnresolvedSet.
Wed, Dec 8, 4:45 PM · Restricted Project
dexonsmith updated the diff for D115386: AST: Avoid using SmallVector::set_size() in UnresolvedSet.

Adding a couple of other possible reviewers.

Wed, Dec 8, 4:38 PM · Restricted Project
dexonsmith updated the diff for D115389: Support: Avoid SmallVector::set_size() in Unix code.

Adding a couple of potential reviewers.

Wed, Dec 8, 4:33 PM · Restricted Project
dexonsmith added reviewers for D115390: Support: Avoid SmallVector::set_size() in Windows code: mstorsjo, aganea, rnk.

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.

Wed, Dec 8, 4:29 PM · Restricted Project
dexonsmith added a comment to D115391: Support: Avoid using SmallVector::set_size().

Probably worth committing separately in case either causes breakage.

Wed, Dec 8, 4:24 PM · Restricted Project
dexonsmith committed rGcd7bc0e010a3: Support: Avoid using SmallVector::set_size() in zlib (authored by dexonsmith).
Support: Avoid using SmallVector::set_size() in zlib
Wed, Dec 8, 4:23 PM
dexonsmith committed rG7df18557791e: Support: Avoid using SmallVector::set_size() in sys::path (authored by dexonsmith).
Support: Avoid using SmallVector::set_size() in sys::path
Wed, Dec 8, 4:23 PM
dexonsmith closed D115391: Support: Avoid using SmallVector::set_size().
Wed, Dec 8, 4:22 PM · Restricted Project
dexonsmith committed rG9911589f5d4a: ADT: Make StringRef::size() and StringRef::empty() constexpr (authored by dexonsmith).
ADT: Make StringRef::size() and StringRef::empty() constexpr
Wed, Dec 8, 4:15 PM
dexonsmith closed D115395: ADT: Make StringRef::size() and StringRef::empty() constexpr.
Wed, Dec 8, 4:15 PM · Restricted Project
dexonsmith added inline comments to D115374: [NFC][clang] Return std::strings built from raw_string_ostreams more efficiently.
Wed, Dec 8, 4:06 PM · Restricted Project
dexonsmith added a comment to D115374: [NFC][clang] Return std::strings built from raw_string_ostreams more efficiently.

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().

Wed, Dec 8, 4:00 PM · Restricted Project
dexonsmith resigned from D115183: [clang][HeaderSearch] Support framework includes in suggestPath....
Wed, Dec 8, 3:30 PM · Restricted Project
dexonsmith accepted D115332: [clang][deps] Use lock_guard instead of unique_lock.

LGTM

Wed, Dec 8, 3:29 PM · Restricted Project
dexonsmith committed rGcfd1d49dc0cc: OpenMP: Avoid using SmallVector::set_size() (authored by dexonsmith).
OpenMP: Avoid using SmallVector::set_size()
Wed, Dec 8, 3:24 PM
dexonsmith closed D115378: OpenMP: Avoid using SmallVector::set_size().
Wed, Dec 8, 3:24 PM · Restricted Project, Restricted Project
dexonsmith added a comment to D115346: [clang][deps] Squash caches for original and minimized files.

This looks really nice.

Wed, Dec 8, 2:50 PM · Restricted Project, Restricted Project
dexonsmith added a comment to D115380: ADT: Make SmallVector::set_size() private.

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.

Wed, Dec 8, 2:08 PM · Restricted Project
dexonsmith committed rGca451d3fa40e: ADT: Add SmallVectorImpl::truncate() to replace uses of set_size() (authored by dexonsmith).
ADT: Add SmallVectorImpl::truncate() to replace uses of set_size()
Wed, Dec 8, 1:58 PM
dexonsmith closed D115383: ADT: Add SmallVectorImpl::truncate() to replace uses of set_size().
Wed, Dec 8, 1:57 PM · Restricted Project
dexonsmith added inline comments to D115380: ADT: Make SmallVector::set_size() private.
Wed, Dec 8, 1:52 PM · Restricted Project
dexonsmith requested review of D115397: Support: Extract sys::fs::readNativeFileToEOF() from MemoryBuffer.
Wed, Dec 8, 1:48 PM · Restricted Project
dexonsmith requested review of D115395: ADT: Make StringRef::size() and StringRef::empty() constexpr.
Wed, Dec 8, 1:43 PM · Restricted Project
dexonsmith added a comment to D115383: ADT: Add SmallVectorImpl::truncate() to replace uses of set_size().

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

Wed, Dec 8, 1:24 PM · Restricted Project
dexonsmith added a comment to D115384: Support: Avoid using SmallVector::set_size() in MemoryBuffer.

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

Wed, Dec 8, 1:23 PM · Restricted Project
dexonsmith updated the diff for D115380: ADT: Make SmallVector::set_size() private.

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

Wed, Dec 8, 1:19 PM · Restricted Project
dexonsmith retitled D115380: ADT: Make SmallVector::set_size() private from WIP: ADT: Make SmallVector::set_size() private to ADT: Make SmallVector::set_size() private.
Wed, Dec 8, 1:15 PM · Restricted Project
dexonsmith requested review of D115391: Support: Avoid using SmallVector::set_size().
Wed, Dec 8, 1:11 PM · Restricted Project
dexonsmith set the repository for D115390: Support: Avoid SmallVector::set_size() in Windows code to rG LLVM Github Monorepo.
Wed, Dec 8, 1:09 PM · Restricted Project
dexonsmith requested review of D115390: Support: Avoid SmallVector::set_size() in Windows code.
Wed, Dec 8, 1:08 PM · Restricted Project
dexonsmith requested review of D115389: Support: Avoid SmallVector::set_size() in Unix code.
Wed, Dec 8, 1:06 PM · Restricted Project
dexonsmith requested review of D115386: AST: Avoid using SmallVector::set_size() in UnresolvedSet.
Wed, Dec 8, 1:02 PM · Restricted Project
dexonsmith updated the summary of D115384: Support: Avoid using SmallVector::set_size() in MemoryBuffer.
Wed, Dec 8, 12:58 PM · Restricted Project
dexonsmith requested review of D115384: Support: Avoid using SmallVector::set_size() in MemoryBuffer.
Wed, Dec 8, 12:56 PM · Restricted Project
dexonsmith requested review of D115383: ADT: Add SmallVectorImpl::truncate() to replace uses of set_size().
Wed, Dec 8, 12:53 PM · Restricted Project
dexonsmith committed rG36e7b8dd564b: ADT: Reduce nesting in resize_for_overwrite(), NFC (authored by dexonsmith).
ADT: Reduce nesting in resize_for_overwrite(), NFC
Wed, Dec 8, 12:52 PM
dexonsmith requested review of D115382: ADT: Avoid using SmallVector::set_size() in SmallString.
Wed, Dec 8, 12:47 PM · Restricted Project
dexonsmith requested review of D115380: ADT: Make SmallVector::set_size() private.
Wed, Dec 8, 12:43 PM · Restricted Project
dexonsmith requested review of D115379: ASTMatchers: Avoid using SmallVector::set_size().
Wed, Dec 8, 12:39 PM · Restricted Project
dexonsmith requested review of D115378: OpenMP: Avoid using SmallVector::set_size().
Wed, Dec 8, 12:35 PM · Restricted Project, Restricted Project
dexonsmith accepted D115043: [clang][deps] Use MemoryBuffer in minimizing FS.

LGTM.

Wed, Dec 8, 10:12 AM · Restricted Project, Restricted Project
dexonsmith accepted D115331: [llvm] Add null-termination capability to SmallVectorMemoryBuffer.

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

Wed, Dec 8, 10:11 AM · Restricted Project, Restricted Project, Restricted Project

Yesterday

dexonsmith added a comment to D115254: Use VersionTuple for parsing versions in Triple.

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.

Tue, Dec 7, 9:39 AM · Restricted Project, Restricted Project

Mon, Dec 6

dexonsmith added inline comments to D114966: [clang][deps] Split filesystem caches.
Mon, Dec 6, 6:12 PM · Restricted Project, Restricted Project
dexonsmith added inline comments to D115043: [clang][deps] Use MemoryBuffer in minimizing FS.
Mon, Dec 6, 4:36 PM · Restricted Project, Restricted Project
dexonsmith accepted D115044: [llvm] Remove out-of-date fixme from SmallVectorMemoryBuffer.

LGTM.

Mon, Dec 6, 4:02 PM · Restricted Project

Thu, Dec 2

dexonsmith requested changes to D114968: [clang][deps] Avoid reading file for stat calls.

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.

Thu, Dec 2, 1:58 PM · Restricted Project
dexonsmith added a comment to D114966: [clang][deps] Split filesystem caches.

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.
Thu, Dec 2, 1:55 PM · Restricted Project, Restricted Project

Wed, Dec 1

dexonsmith accepted D113812: [Cloning] Clone metadata on function declarations.

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

Since D21052

Wed, Dec 1, 3:31 PM · Restricted Project
dexonsmith added a comment to D114860: [llvm-c] Make LLVMAddAlias opaque pointer compatible.

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.

Wed, Dec 1, 3:03 PM · Restricted Project

Mon, Nov 29

dexonsmith added a comment to D114095: [clang][lex] Include tracking: simplify and move to preprocessor.

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).

Mon, Nov 29, 1:36 PM · Restricted Project
dexonsmith updated subscribers of D109459: [libc++][ABI BREAK] Do not use the C++03 emulation for std::nullptr_t by default.

@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.

Mon, Nov 29, 12:56 PM · Restricted Project

Fri, Nov 19

dexonsmith added a reviewer for D114163: Use VersionTuple for parsing versions in Triple. This makes it possible to distinguish between "16" and "16.0" after parsing, which previously was not possible.: arphaman.

@arphaman , can you take a look?

Fri, Nov 19, 1:15 PM · Restricted Project, Restricted Project

Wed, Nov 17

dexonsmith added a comment to D114120: [clang] Remove CLANG_ROUND_TRIP_CC1_ARGS and always roundtrip in +assert builds.

SGTM; I'll let @jansvoboda11 confirm though.

Wed, Nov 17, 1:49 PM · Restricted Project
dexonsmith added a comment to D104827: [DebugInfo] Enforce implicit constraints on `distinct` MDNodes.

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.

Wed, Nov 17, 1:23 PM · Restricted Project
dexonsmith added inline comments to D112915: [clang][modules] Track included files per submodule.
Wed, Nov 17, 1:05 PM · Restricted Project, Restricted Project
dexonsmith accepted D114092: [clang][lex] NFC: Remove unused HeaderFileInfo member.

LGTM.

Wed, Nov 17, 12:56 PM · Restricted Project
dexonsmith accepted D114096: [clang][lex][modules] Stop tracking number of includes.
Wed, Nov 17, 12:55 PM · Restricted Project
dexonsmith added a comment to D114096: [clang][lex][modules] Stop tracking number of includes.

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

Wed, Nov 17, 12:54 PM · Restricted Project
dexonsmith added a comment to D114095: [clang][lex] Include tracking: simplify and move to preprocessor.

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.

Wed, Nov 17, 12:51 PM · Restricted Project
dexonsmith accepted D114093: [clang][lex] Refactor check for the first file include.

LGTM with one nit inline.

Wed, Nov 17, 12:43 PM · Restricted Project

Tue, Nov 16

dexonsmith added inline comments to D104827: [DebugInfo] Enforce implicit constraints on `distinct` MDNodes.
Tue, Nov 16, 6:36 PM · Restricted Project
dexonsmith added inline comments to D113511: [mlir] Optimize usage of llvm::mapped_iterator.
Tue, Nov 16, 5:59 PM · Restricted Project, Restricted Project
dexonsmith committed rGd00256bac07e: ADT: Adding a key_type definition to MapVector (authored by kevcadieux).
ADT: Adding a key_type definition to MapVector
Tue, Nov 16, 5:41 PM
dexonsmith closed D113242: Adding a key_type definition to MapVector.
Tue, Nov 16, 5:40 PM · Restricted Project
dexonsmith added inline comments to D113511: [mlir] Optimize usage of llvm::mapped_iterator.
Tue, Nov 16, 3:01 PM · Restricted Project, Restricted Project
dexonsmith committed rG77898a4c0c8c: Coverage: Fix iterated type for LineCoverageIterator (authored by dexonsmith).
Coverage: Fix iterated type for LineCoverageIterator
Tue, Nov 16, 2:40 PM
dexonsmith added a comment to D113958: DebugInfo: Make DWARFExpression::iterator a const iterator.

Thanks for the reviews! Pushed in fd6018072ace7d5cdf537fd63a44c050a98e52fc.

Tue, Nov 16, 10:29 AM · Restricted Project
dexonsmith committed rGfd6018072ace: DebugInfo: Make DWARFExpression::iterator a const iterator (authored by dexonsmith).
DebugInfo: Make DWARFExpression::iterator a const iterator
Tue, Nov 16, 10:26 AM
dexonsmith closed D113958: DebugInfo: Make DWARFExpression::iterator a const iterator.
Tue, Nov 16, 10:26 AM · Restricted Project
dexonsmith committed rGa0f1f171314f: DebugInfo: Stop modifying Operation::Error inside of verify() (authored by dexonsmith).
DebugInfo: Stop modifying Operation::Error inside of verify()
Tue, Nov 16, 10:21 AM
dexonsmith closed D113957: DebugInfo: Stop modifying Operation::Error inside of verify().
Tue, Nov 16, 10:21 AM · Restricted Project

Mon, Nov 15

dexonsmith added a comment to D113957: DebugInfo: Stop modifying Operation::Error inside of verify().

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.
Mon, Nov 15, 5:48 PM · Restricted Project
dexonsmith added a comment to D113934: [llvm] adapt DWARFExpression.h for 6b9b86db9dd974585a5c71cf2e5231d1e3385f7c.

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.

Mon, Nov 15, 5:43 PM · Restricted Project
dexonsmith updated the diff for D113958: DebugInfo: Make DWARFExpression::iterator a const iterator.

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

Mon, Nov 15, 5:41 PM · Restricted Project
dexonsmith requested review of D113958: DebugInfo: Make DWARFExpression::iterator a const iterator.
Mon, Nov 15, 5:39 PM · Restricted Project
dexonsmith requested review of D113957: DebugInfo: Stop modifying Operation::Error inside of verify().
Mon, Nov 15, 5:37 PM · Restricted Project
dexonsmith committed rGd7a81359d781: DebugInfo: Make DWARFExpression::iterator::skipBytes() const, NFC (authored by dexonsmith).
DebugInfo: Make DWARFExpression::iterator::skipBytes() const, NFC
Mon, Nov 15, 5:33 PM
dexonsmith committed rG79df41011ba6: DebugInfo: const-qualify accessors of DWARFExpression::Operation (authored by dexonsmith).
DebugInfo: const-qualify accessors of DWARFExpression::Operation
Mon, Nov 15, 5:32 PM
dexonsmith committed rGb23ba295bd12: DebugInfo: Make DWARFExpression::iterator::operator++ return itself (authored by dexonsmith).
DebugInfo: Make DWARFExpression::iterator::operator++ return itself
Mon, Nov 15, 5:29 PM
dexonsmith added inline comments to D113934: [llvm] adapt DWARFExpression.h for 6b9b86db9dd974585a5c71cf2e5231d1e3385f7c.
Mon, Nov 15, 1:13 PM · Restricted Project
dexonsmith added inline comments to D113934: [llvm] adapt DWARFExpression.h for 6b9b86db9dd974585a5c71cf2e5231d1e3385f7c.
Mon, Nov 15, 1:12 PM · Restricted Project
dexonsmith added inline comments to D113912: [JITLink] Fix splitBlock if there are symbols span across the boundary.
Mon, Nov 15, 10:35 AM · Restricted Project

Fri, Nov 12

dexonsmith accepted D113242: Adding a key_type definition to MapVector.

LGTM.

Fri, Nov 12, 9:43 PM · Restricted Project
dexonsmith accepted D109128: [VFS] Use original path when falling back to external FS.

@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.

Fri, Nov 12, 9:41 PM · Restricted Project, Restricted Project
dexonsmith added a comment to D113812: [Cloning] Clone metadata on function declarations.

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.

Fri, Nov 12, 9:24 PM · Restricted Project
dexonsmith committed rG79c5479822e6: Support: Pass wrapped Error's error code through FileError (authored by dexonsmith).
Support: Pass wrapped Error's error code through FileError
Fri, Nov 12, 9:20 PM
dexonsmith closed D113225: Support: Pass wrapped Error's error code through FileError.
Fri, Nov 12, 9:20 PM · Restricted Project
dexonsmith closed D113799: ADT: Fix const-correctness of iterator facade.
Fri, Nov 12, 8:45 PM · Restricted Project
dexonsmith committed rG6b9b86db9dd9: ADT: Fix const-correctness of iterator facade (authored by dexonsmith).
ADT: Fix const-correctness of iterator facade
Fri, Nov 12, 8:45 PM
dexonsmith added a comment to D113799: ADT: Fix const-correctness of iterator facade.

Thanks for the reviews! Pushed in 6b9b86db9dd974585a5c71cf2e5231d1e3385f7c.

Fri, Nov 12, 8:44 PM · Restricted Project
dexonsmith committed rG75c86c993592: Support: Make VarStreamArrayIterator iterate over const values (authored by dexonsmith).
Support: Make VarStreamArrayIterator iterate over const values
Fri, Nov 12, 8:38 PM
dexonsmith closed D113797: Support: Make VarStreamArrayIterator iterate over const values.
Fri, Nov 12, 8:38 PM · Restricted Project
dexonsmith updated subscribers of rGc3edab8f781d: ADT: Avoid repeating iterator adaptor/facade template params, NFC.

@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
Fri, Nov 12, 3:15 PM