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 (261 w, 2 d)

Recent Activity

Yesterday

dexonsmith edited reviewers for D59628: Add support for __attribute__((objc_class_stub)), added: erik.pilkington, arphaman; removed: dexonsmith.

+Erik and Alex, can you take a look at this?

Wed, Mar 20, 7:51 PM · Restricted Project
dexonsmith accepted D59489: [libc++][CMake] Clean up some of the libc++ re-exporting logic.

LGTM.

Wed, Mar 20, 10:42 AM

Mon, Mar 18

dexonsmith accepted D58548: IR: Support parsing numeric block ids, and emit them in textual output..

LGTM!

Mon, Mar 18, 3:25 PM · Restricted Project, Restricted Project
dexonsmith updated the diff for D59388: Basic: Return a reference from FileManager::getVirtualFileSystem, NFC.

Rebase.

Mon, Mar 18, 9:03 AM
dexonsmith added a comment to D59377: Frontend: Remove CompilerInstance::VirtualFileSystem, NFC.

... since I noticed that FileManager ...

This kind of implies that we should move the comment from FileManager constructor implementation to the header thus making it an explicit part of the interface contract.

// If the caller doesn't provide a virtual file system, just grab the real
// file system.

Maybe https://reviews.llvm.org/D59388 would be the right place to do it.

Mon, Mar 18, 9:00 AM
dexonsmith updated the diff for D59377: Frontend: Remove CompilerInstance::VirtualFileSystem, NFC.

Document the default VFS used by the FileManager.

Mon, Mar 18, 9:00 AM
dexonsmith added a comment to D59388: Basic: Return a reference from FileManager::getVirtualFileSystem, NFC.

Hi Duncan, thanks for working on better interfaces in clang!

I am just wondering - is it safe to have the lifetime of a single object on heap managed by two different IntrusiveRefCntPtr instances?

Mon, Mar 18, 8:50 AM

Thu, Mar 14

dexonsmith added a parent revision for D59388: Basic: Return a reference from FileManager::getVirtualFileSystem, NFC: D59377: Frontend: Remove CompilerInstance::VirtualFileSystem, NFC.
Thu, Mar 14, 2:49 PM
dexonsmith added a child revision for D59377: Frontend: Remove CompilerInstance::VirtualFileSystem, NFC: D59388: Basic: Return a reference from FileManager::getVirtualFileSystem, NFC.
Thu, Mar 14, 2:49 PM
dexonsmith created D59388: Basic: Return a reference from FileManager::getVirtualFileSystem, NFC.
Thu, Mar 14, 2:49 PM
dexonsmith updated the diff for D59377: Frontend: Remove CompilerInstance::VirtualFileSystem, NFC.

I slightly simplified the code for CompilerInstance::createFileManager (replaced with an assert), since I noticed that FileManager always constructs its own VFS if it isn't passed one.

Thu, Mar 14, 2:25 PM
dexonsmith added reviewers for D59377: Frontend: Remove CompilerInstance::VirtualFileSystem, NFC: Bigcheese, arphaman, bruno.
Thu, Mar 14, 1:07 PM
dexonsmith created D59377: Frontend: Remove CompilerInstance::VirtualFileSystem, NFC.
Thu, Mar 14, 11:30 AM

Wed, Mar 13

dexonsmith added a comment to D58514: Avoid needlessly copying blocks that initialize or are assigned to local auto variables to the heap.

Sorry, I misread the code. If you change the parameter type of bar to BlockTy, the code crashes. If the type is id, it doesn't crash because IRGen copies the block to the heap in foo before passing it to bar.

Wed, Mar 13, 5:19 PM · Restricted Project
dexonsmith added a comment to D58514: Avoid needlessly copying blocks that initialize or are assigned to local auto variables to the heap.

That code doesn't crash because the block is retained at the entry of bar and ARC optimizer doesn't remove the retain/release pairs in bar.

Wed, Mar 13, 4:33 PM · Restricted Project
dexonsmith added a comment to D58514: Avoid needlessly copying blocks that initialize or are assigned to local auto variables to the heap.

Seems like the chromium code is valid and shouldn't crash. John/Erik what do you think? The following code also crashes with this patch applied.

typedef void (^BlockTy)();

BlockTy sb;
__weak BlockTy wb;

void foo(id a) {
  auto b = ^{ NSLog(@"foo %@", a); };
  wb = b; // block isn't copied to the heap.
  sb = b; // block is copied to the heap.
}

int main() {
  auto x = [NSObject new];
  foo(x);
  sb();
  wb();
  return 0;
}
Wed, Mar 13, 4:21 PM · Restricted Project

Tue, Mar 12

dexonsmith committed rG70d759b4eb95: Modules: Add LangOptions::CacheGeneratedPCH (authored by dexonsmith).
Modules: Add LangOptions::CacheGeneratedPCH
Tue, Mar 12, 11:39 AM
dexonsmith committed rL355950: Modules: Add LangOptions::CacheGeneratedPCH.
Modules: Add LangOptions::CacheGeneratedPCH
Tue, Mar 12, 11:39 AM
dexonsmith committed rC355950: Modules: Add LangOptions::CacheGeneratedPCH.
Modules: Add LangOptions::CacheGeneratedPCH
Tue, Mar 12, 11:38 AM
dexonsmith closed D59176: Modules: Add LangOptions::CacheGeneratedPCH.

Committed in r355950. Thanks for the review.

Tue, Mar 12, 11:38 AM

Mon, Mar 11

dexonsmith added inline comments to D59176: Modules: Add LangOptions::CacheGeneratedPCH.
Mon, Mar 11, 8:35 PM
dexonsmith updated the diff for D59176: Modules: Add LangOptions::CacheGeneratedPCH.

Updated the constructor call to PCHGenerator in GenerateModuleAction::CreateASTConsumer to use BuildingImplicitModule on its own. Checking where it's set (only in compileModuleImpl), it's exactly the condition we want here.

Mon, Mar 11, 8:25 PM
dexonsmith added a comment to D59176: Modules: Add LangOptions::CacheGeneratedPCH.

This commit by itself doesn't change any behavior, right?

Mon, Mar 11, 1:56 PM

Sat, Mar 9

dexonsmith committed rGb7db2e9f8242: Stop relying on allocator behaviour in modules unit test (authored by dexonsmith).
Stop relying on allocator behaviour in modules unit test
Sat, Mar 9, 12:15 PM
dexonsmith committed rC355780: Stop relying on allocator behaviour in modules unit test.
Stop relying on allocator behaviour in modules unit test
Sat, Mar 9, 12:14 PM
dexonsmith committed rL355780: Stop relying on allocator behaviour in modules unit test.
Stop relying on allocator behaviour in modules unit test
Sat, Mar 9, 12:14 PM
dexonsmith committed rG2fd0d227f6bc: Fix slashes in path references in -Rmodule-import test from r355778 (authored by dexonsmith).
Fix slashes in path references in -Rmodule-import test from r355778
Sat, Mar 9, 11:33 AM
dexonsmith committed rC355779: Fix slashes in path references in -Rmodule-import test from r355778.
Fix slashes in path references in -Rmodule-import test from r355778
Sat, Mar 9, 11:32 AM
dexonsmith committed rL355779: Fix slashes in path references in -Rmodule-import test from r355778.
Fix slashes in path references in -Rmodule-import test from r355778
Sat, Mar 9, 11:32 AM
dexonsmith closed D58893: Modules: Invalidate out-of-date PCMs as they're discovered.

Committed in r355778.

Sat, Mar 9, 9:44 AM
dexonsmith committed rG0a2be46cfdb6: Modules: Invalidate out-of-date PCMs as they're discovered (authored by dexonsmith).
Modules: Invalidate out-of-date PCMs as they're discovered
Sat, Mar 9, 9:44 AM
dexonsmith committed rL355778: Modules: Invalidate out-of-date PCMs as they're discovered.
Modules: Invalidate out-of-date PCMs as they're discovered
Sat, Mar 9, 9:44 AM
dexonsmith committed rC355778: Modules: Invalidate out-of-date PCMs as they're discovered.
Modules: Invalidate out-of-date PCMs as they're discovered
Sat, Mar 9, 9:44 AM
dexonsmith committed rC355777: Modules: Rename MemoryBufferCache to InMemoryModuleCache.
Modules: Rename MemoryBufferCache to InMemoryModuleCache
Sat, Mar 9, 9:34 AM
dexonsmith committed rG8bef5cd49a8b: Modules: Rename MemoryBufferCache to InMemoryModuleCache (authored by dexonsmith).
Modules: Rename MemoryBufferCache to InMemoryModuleCache
Sat, Mar 9, 9:34 AM
dexonsmith closed D58890: Modules: Rename MemoryBufferCache to InMemoryModuleCache.

Committed in r355777.

Sat, Mar 9, 9:34 AM
dexonsmith committed rL355777: Modules: Rename MemoryBufferCache to InMemoryModuleCache.
Modules: Rename MemoryBufferCache to InMemoryModuleCache
Sat, Mar 9, 9:33 AM
dexonsmith added a child revision for D58893: Modules: Invalidate out-of-date PCMs as they're discovered: D59176: Modules: Add LangOptions::CacheGeneratedPCH.
Sat, Mar 9, 9:23 AM
dexonsmith added a parent revision for D59176: Modules: Add LangOptions::CacheGeneratedPCH: D58893: Modules: Invalidate out-of-date PCMs as they're discovered.
Sat, Mar 9, 9:23 AM
dexonsmith created D59176: Modules: Add LangOptions::CacheGeneratedPCH.
Sat, Mar 9, 9:23 AM

Thu, Mar 7

dexonsmith added inline comments to D59112: [Bitcode] Fix bitcode compatibility issue with clang.arc.use intrinsic.
Thu, Mar 7, 7:42 PM · Restricted Project
dexonsmith accepted D59112: [Bitcode] Fix bitcode compatibility issue with clang.arc.use intrinsic.

LGTM too with some nitpicks (forgot to hit "submit" earlier this afternoon).

Thu, Mar 7, 7:29 PM · Restricted Project
dexonsmith resigned from D59080: Merge of global constants not happenning.

+ab, do you still know this code?

Thu, Mar 7, 10:25 AM
dexonsmith added inline comments to D59093: [libc++] Mark <filesystem> as unavailable on Apple platforms.
Thu, Mar 7, 10:25 AM

Wed, Mar 6

dexonsmith accepted D59032: Passthrough compiler launcher.

LGTM. Thanks!

Wed, Mar 6, 11:33 AM · Restricted Project

Tue, Mar 5

dexonsmith committed rGa75c4df5242f: Fix slashes in path references in -Rmodule-import test from r355477 (authored by dexonsmith).
Fix slashes in path references in -Rmodule-import test from r355477
Tue, Mar 5, 9:43 PM
dexonsmith committed rC355482: Fix slashes in path references in -Rmodule-import test from r355477.
Fix slashes in path references in -Rmodule-import test from r355477
Tue, Mar 5, 9:43 PM
dexonsmith committed rL355482: Fix slashes in path references in -Rmodule-import test from r355477.
Fix slashes in path references in -Rmodule-import test from r355477
Tue, Mar 5, 9:43 PM
dexonsmith committed rG9dda8f540c8e: Modules: Add -Rmodule-import (authored by dexonsmith).
Modules: Add -Rmodule-import
Tue, Mar 5, 6:51 PM
dexonsmith committed rC355477: Modules: Add -Rmodule-import.
Modules: Add -Rmodule-import
Tue, Mar 5, 6:51 PM
dexonsmith committed rL355477: Modules: Add -Rmodule-import.
Modules: Add -Rmodule-import
Tue, Mar 5, 6:51 PM
dexonsmith closed D58891: Modules: Add -Rmodule-import.

Committed in r355477.

Tue, Mar 5, 6:51 PM

Mon, Mar 4

dexonsmith added a comment to D58890: Modules: Rename MemoryBufferCache to InMemoryModuleCache.

InMemoryModuleCache seems like a way more appropriate name here. Also thanks for improving some of the comments.

Because of the move to Serialization we can no longer abuse the Preprocessor to forward it to the ASTReader. Besides the rename and file move, that means Preprocessor::Preprocessor has one fewer parameter and ASTReader::ASTReader has one more.

Are there any C api bits that expose preprocessor stuff and also need updates?

Mon, Mar 4, 4:32 PM

Sun, Mar 3

dexonsmith added inline comments to D58891: Modules: Add -Rmodule-import.
Sun, Mar 3, 11:29 PM
dexonsmith added parent revisions for D58893: Modules: Invalidate out-of-date PCMs as they're discovered: D58890: Modules: Rename MemoryBufferCache to InMemoryModuleCache, D58891: Modules: Add -Rmodule-import.
Sun, Mar 3, 11:20 PM
dexonsmith created D58893: Modules: Invalidate out-of-date PCMs as they're discovered.
Sun, Mar 3, 11:20 PM
dexonsmith added a child revision for D58891: Modules: Add -Rmodule-import: D58893: Modules: Invalidate out-of-date PCMs as they're discovered.
Sun, Mar 3, 11:20 PM
dexonsmith added a child revision for D58890: Modules: Rename MemoryBufferCache to InMemoryModuleCache: D58893: Modules: Invalidate out-of-date PCMs as they're discovered.
Sun, Mar 3, 11:20 PM
dexonsmith created D58891: Modules: Add -Rmodule-import.
Sun, Mar 3, 9:56 PM
dexonsmith created D58890: Modules: Rename MemoryBufferCache to InMemoryModuleCache.
Sun, Mar 3, 9:51 PM
dexonsmith committed rGfae03d8add5d: Modules: Document that ReadASTCore exits its final loop via `return`, NFC (authored by dexonsmith).
Modules: Document that ReadASTCore exits its final loop via `return`, NFC
Sun, Mar 3, 12:17 PM
dexonsmith committed rC355294: Modules: Document that ReadASTCore exits its final loop via `return`, NFC.
Modules: Document that ReadASTCore exits its final loop via `return`, NFC
Sun, Mar 3, 12:17 PM
dexonsmith committed rL355294: Modules: Document that ReadASTCore exits its final loop via `return`, NFC.
Modules: Document that ReadASTCore exits its final loop via `return`, NFC
Sun, Mar 3, 12:17 PM

Mon, Feb 25

dexonsmith accepted D58631: [ThinLTO] Use defined node and edge order when dumping DOT files.
Mon, Feb 25, 10:12 AM · Restricted Project

Sat, Feb 23

dexonsmith committed rGe7b9464943e1: VFS: Avoid some unnecessary std::string copies (authored by dexonsmith).
VFS: Avoid some unnecessary std::string copies
Sat, Feb 23, 3:49 PM
dexonsmith committed rL354739: VFS: Avoid some unnecessary std::string copies.
VFS: Avoid some unnecessary std::string copies
Sat, Feb 23, 3:48 PM

Fri, Feb 22

dexonsmith added inline comments to D58418: [clang][DirectoryWatcher] Upstream DirectoryWatcher.
Fri, Feb 22, 6:37 PM · Restricted Project
dexonsmith added a comment to D58559: emit '(assertions enabled)' in the version string for a build of clang with assertions enabled.

I like this. Can you start a discussion on cfe-dev (if you haven't already)? This is user-visible so I want to be sure other vendors are happy with this; if not, we can hide it behind a CMake flag.

Fri, Feb 22, 5:02 PM · Restricted Project
dexonsmith added a comment to D58548: IR: Support parsing numeric block ids, and emit them in textual output..

I have a few nitpicks, but otherwise this seems right. I'll wait for the llvm-dev discussion before LGTM'ing though.

Fri, Feb 22, 1:34 PM · Restricted Project, Restricted Project
dexonsmith added a comment to D58548: IR: Support parsing numeric block ids, and emit them in textual output..

+1. Is there any reason not to use "%4" in the definition?

define i32 @f(i32, i32) {
  %3 = add i32 %0, %1
  br label %4

%4:
  ret i32 %3
}

Maybe it creates an ambiguity in the grammar or something.

A % isn't used for labels that do have names, so David's approach seems consistent.

Fri, Feb 22, 11:43 AM · Restricted Project, Restricted Project
dexonsmith added a comment to D58548: IR: Support parsing numeric block ids, and emit them in textual output..

+1. Is there any reason not to use "%4" in the definition?

define i32 @f(i32, i32) {
  %3 = add i32 %0, %1
  br label %4

%4:
  ret i32 %3
}

Maybe it creates an ambiguity in the grammar or something.

Fri, Feb 22, 11:43 AM · Restricted Project, Restricted Project
dexonsmith added a comment to D20582: Don't add repeats of llvm.ident list when linking.

It’s unfortunate that this will be quadratic in the number of modules. Is there a way we could improve that?

Well the linker API only exposes linking one module into one other at at time, so I don't know. Currently we have the AMDGPUUnifyMetadata pass as a workaround which cleans these up in a pass over the fully linked module, so that avoids revisiting for each module but it would make more sense if the linker dealt with this.

Fri, Feb 22, 9:55 AM
dexonsmith added a comment to D20582: Don't add repeats of llvm.ident list when linking.

It’s unfortunate that this will be quadratic in the number of modules. Is there a way we could improve that?

Fri, Feb 22, 8:00 AM
dexonsmith added a comment to D58548: IR: Support parsing numeric block ids, and emit them in textual output..

I like this idea, and I don’t think the textual IR central is too important.

Fri, Feb 22, 7:51 AM · Restricted Project, Restricted Project
dexonsmith added a comment to D58548: IR: Support parsing numeric block ids, and emit them in textual output..

I like this idea, and I don’t think the textual IR central is too important. A few things:

Fri, Feb 22, 7:51 AM · Restricted Project, Restricted Project

Wed, Feb 20

dexonsmith added inline comments to D58098: [IR] Add Use::moveToFrontOfUseList() method.
Wed, Feb 20, 10:19 AM · Restricted Project

Tue, Feb 19

dexonsmith added inline comments to D58098: [IR] Add Use::moveToFrontOfUseList() method.
Tue, Feb 19, 6:25 PM · Restricted Project
dexonsmith added inline comments to D58421: Add partial implementation of std::to_address() as llvm::to_address().
Tue, Feb 19, 6:06 PM · Restricted Project
dexonsmith added inline comments to D58098: [IR] Add Use::moveToFrontOfUseList() method.
Tue, Feb 19, 5:38 PM · Restricted Project
dexonsmith added a comment to D58098: [IR] Add Use::moveToFrontOfUseList() method.

This seems like it could be useful. Do you have specific places in mind where you'd take advantage of it?

Tue, Feb 19, 3:30 PM · Restricted Project

Feb 19 2019

dexonsmith added a comment to D58384: [ThinLTO] Fix test with reverse-iteration.

Summary clusters, nodes and edges are retrieved from DenseMap with arbitrary order, which doesn't make result DOT file invalid.
Reverse iteration just exposes test issues.

Feb 19 2019, 9:29 AM · Restricted Project

Feb 14 2019

dexonsmith added a comment to D58028: Always compare C++ typeinfo (based on libstdc++ implementation)..

I’d like @kubamracek and/or @Bigcheese to take a look first.

Feb 14 2019, 6:36 AM · Restricted Project, Restricted Project

Feb 11 2019

dexonsmith added reviewers for D58028: Always compare C++ typeinfo (based on libstdc++ implementation).: Bigcheese, kubamracek.
Feb 11 2019, 6:07 AM · Restricted Project, Restricted Project

Feb 7 2019

dexonsmith added inline comments to D57761: [libc++] Avoid UB in the no-exceptions mode in a few places.
Feb 7 2019, 3:41 PM · Restricted Project

Feb 5 2019

dexonsmith added reviewers for D57797: Variable auto-init: fix __block initialization: ahatanak, erik.pilkington.
Feb 5 2019, 5:17 PM · Restricted Project
dexonsmith added inline comments to D57761: [libc++] Avoid UB in the no-exceptions mode in a few places.
Feb 5 2019, 10:03 AM · Restricted Project

Feb 4 2019

dexonsmith added a comment to D57688: [libcxx] Remove <ext/hash_set>, <ext/hash_map> and <ext/__hash>.

Oh, I agree. However, when I have proposed this in the past, I have gotten pushback from many people (including Apple, Google and others).
At the very least, you should announce this on llvm-dev, and get feedback there.

Feb 4 2019, 11:20 AM
dexonsmith added a comment to D56485: Always compare C++ typeinfo (based on libstdc++ implementation)..

Hi, there's no reply from the people who were added to this review.
May I understand that as ready to be installed?

Feb 4 2019, 9:29 AM

Jan 24 2019

dexonsmith updated subscribers of D56485: Always compare C++ typeinfo (based on libstdc++ implementation)..

I would suggest to revert the commit, and submit the new differential with properly subscribed lists this time.
At least that is the 'recommended' general practice.

Done.

Jan 24 2019, 9:51 AM

Jan 17 2019

dexonsmith added a reviewer for D56805: mac: Correctly disable tools/lto tests when building with LLVM_ENABLE_PIC=OFF: steven_wu.
Jan 17 2019, 2:18 PM

Jan 16 2019

dexonsmith added a comment to D56816: [ObjC] Follow-up r350768 and allow the use of unavailable methods that are declared in a parent class from within the @implementation context.

My understanding is that rL349841 accidentally started producing some spurious warnings/errors, rL350768 fixed some instances, and this change fixes more instances. Given that the first two changes are already in the 8.0 branch, should this be cherry-picked to 8.0 as well once it lands?

Jan 16 2019, 5:19 PM
dexonsmith added a comment to D56802: [CodeGenObjC] Treat ivar offsets variables as constant if they refer to ivars of a direct subclass of NSObject with an @implementation.

We've tossed around the idea of doing things like this before, but I was hoping that it wouldn't have to be specific to NSObject and that we'd e.g. have an attribute that guarantees that the @interface declares all the ivars for a class. Are we still thinking that?

Jan 16 2019, 2:41 PM

Jan 11 2019

dexonsmith added a comment to D56608: [Darwin][Driver] Don't pass a file as object_path_lto during ThinLTO.

The code looks good. Can you add a test too? Might need to require “shell”.

Jan 11 2019, 10:09 AM

Jan 10 2019

dexonsmith added inline comments to D56567: [ADT] Force attribute used on functions marked as always_inline..
Jan 10 2019, 4:04 PM
dexonsmith added inline comments to D56523: Improve a -Wunguarded-availability note.
Jan 10 2019, 11:08 AM

Jan 3 2019

dexonsmith added a comment to D56226: [clang-format] square parens that are followed by '=' are not Objective-C message sends.

Note that a message send needs to have two expressions without an operator in between. Can we also rule out all square brackets that just have a single identifier or number token?

Jan 3 2019, 6:47 AM

Dec 20 2018

dexonsmith added a comment to D55869: Convert some ObjC retain/release msgSends to runtime calls.
In D55869#1337706, @js wrote:

The ObjFW runtime itself does not contain anything about release, retain or autorelease - these are all part of ObjFW (as in the framework). As ObjFW also supports the Apple runtime, as well as mixing with Foundation code, it so far only provides objc_retain and objc_release when they are missing, to make ARC work. They just call into the corresponding methods on the object and do nothing else.

How will this change work from the Apple runtime? Will objc_retain/objc_release call the retain/release on the object if it implements its own? Should I drop my own retain/release implementation from my root object if I am compiling against the Apple runtime?

Dec 20 2018, 9:16 AM
dexonsmith added a comment to D55869: Convert some ObjC retain/release msgSends to runtime calls.

Okay. That's also presumably true for open-source runtimes that support ARC; tagging David Chisnall and Jonathan Schleifer to get their input.

Dec 20 2018, 9:14 AM

Dec 19 2018

dexonsmith added a comment to D53890: [LTO] Record whether LTOUnit splitting is enabled in index.
In D53890#1335770, @pcc wrote:

This means that

clang -c -flto=thin foo.c
[upgrade clang]
clang -c -flto=thin bar.c
clang foo.o bar.o

will fail unless you add -fsplit-lto-unit to the second invocation. Given that the alternative is that we potentially silently miscompile, maybe that's fine. It would be possible to handle this case without aborting or miscompiling by splitting bar.o inside the linker and treating it as if it had always been split, but given that there's a workaround maybe that's not that critical.

Dec 19 2018, 6:23 AM

Dec 18 2018

dexonsmith added inline comments to D55760: [ADT] IntervalMap: add contains(a, b) method.
Dec 18 2018, 1:41 PM