Page MenuHomePhabricator

erik.pilkington (Erik Pilkington)
User

Projects

User does not belong to any projects.

User Details

User Since
Feb 2 2016, 10:56 AM (154 w, 5 d)

Recent Activity

Fri, Jan 18

erik.pilkington accepted D56892: Add a priority field to availability attributes to prioritize explicit attributes from declaration over attributes from '#pragma clang attribute'.

LGTM, thanks!

Fri, Jan 18, 2:56 PM
erik.pilkington added a comment to D56892: Add a priority field to availability attributes to prioritize explicit attributes from declaration over attributes from '#pragma clang attribute'.

This seems pretty reasonable to me. I agree that a more general mechanism to override #pca (/implicit) attributes would be pretty useful, but I guess there is no need to jump the gun on that.

Fri, Jan 18, 1:53 PM
erik.pilkington updated the diff for D56879: [Sema] Suppress a warning about a forward-declared fixed enum in C mode.

Add a -pedantic RUN line.

Fri, Jan 18, 9:56 AM

Thu, Jan 17

erik.pilkington created D56879: [Sema] Suppress a warning about a forward-declared fixed enum in C mode.
Thu, Jan 17, 2:36 PM
erik.pilkington added inline comments to D56855: Add ___Z demangling to new common demangle function.
Thu, Jan 17, 1:44 PM
erik.pilkington added inline comments to D56721: Move llvm-objdump demangle function into demangler library.
Thu, Jan 17, 1:42 PM
erik.pilkington accepted D53538: NFC: Reorganize the demangler a bit.

I think I'll just land this, I have some dependent work that I want to do.

Thu, Jan 17, 12:40 PM
erik.pilkington added inline comments to D56721: Move llvm-objdump demangle function into demangler library.
Thu, Jan 17, 12:32 PM
erik.pilkington added inline comments to D56721: Move llvm-objdump demangle function into demangler library.
Thu, Jan 17, 12:25 PM
erik.pilkington accepted D56855: Add ___Z demangling to new common demangle function.

LGTM, thanks for following up!

Thu, Jan 17, 9:08 AM

Wed, Jan 16

erik.pilkington updated the diff for D56802: [CodeGenObjC] Treat ivar offsets variables as constant if they refer to ivars of a direct subclass of NSObject with an @implementation.

Sure, that makes sense. The new patch makes the global constant again.

Wed, Jan 16, 11:10 PM
erik.pilkington accepted D56816: [ObjC] Follow-up r350768 and allow the use of unavailable methods that are declared in a parent class from within the @implementation context.

LGTM, thanks!

Wed, Jan 16, 8:51 PM
erik.pilkington added inline comments 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.
Wed, Jan 16, 4:24 PM
erik.pilkington updated the diff for D56802: [CodeGenObjC] Treat ivar offsets variables as constant if they refer to ivars of a direct subclass of NSObject with an @implementation.

Address review comments: move the check to a function, and call it when emitting the ivar offset instead of when we emit the global. Thanks!

Wed, Jan 16, 4:12 PM
erik.pilkington created D56802: [CodeGenObjC] Treat ivar offsets variables as constant if they refer to ivars of a direct subclass of NSObject with an @implementation.
Wed, Jan 16, 1:01 PM
erik.pilkington added inline comments to D56721: Move llvm-objdump demangle function into demangler library.
Wed, Jan 16, 10:27 AM
erik.pilkington accepted D56721: Move llvm-objdump demangle function into demangler library.

LGTM, thanks!

Wed, Jan 16, 10:23 AM

Tue, Jan 15

erik.pilkington added a child revision for D56761: Add a parameter to the objectsize intrinsic that controls whether to evaluate size dynamically: D56760: Add a new builtin: __builtin_dynamic_object_size.
Tue, Jan 15, 4:54 PM
erik.pilkington added a parent revision for D56760: Add a new builtin: __builtin_dynamic_object_size: D56761: Add a parameter to the objectsize intrinsic that controls whether to evaluate size dynamically.
Tue, Jan 15, 4:54 PM
erik.pilkington created D56761: Add a parameter to the objectsize intrinsic that controls whether to evaluate size dynamically.
Tue, Jan 15, 4:53 PM
erik.pilkington created D56760: Add a new builtin: __builtin_dynamic_object_size.
Tue, Jan 15, 4:52 PM
erik.pilkington added a comment to D56721: Move llvm-objdump demangle function into demangler library.

Hi, thanks for working on this! This seems like a useful function.

Tue, Jan 15, 10:30 AM

Thu, Jan 10

erik.pilkington added inline comments to D56523: Improve a -Wunguarded-availability note.
Thu, Jan 10, 12:29 PM
erik.pilkington updated the diff for D56523: Improve a -Wunguarded-availability note.

Fix some triples in the tests.

Thu, Jan 10, 12:29 PM
erik.pilkington added inline comments to D56523: Improve a -Wunguarded-availability note.
Thu, Jan 10, 9:42 AM

Wed, Jan 9

erik.pilkington created D56523: Improve a -Wunguarded-availability note.
Wed, Jan 9, 4:51 PM
erik.pilkington requested review of D56405: Split -Wdelete-non-virtual-dtor into two groups.
Wed, Jan 9, 12:38 PM
erik.pilkington accepted D56469: [ObjC] Allow the use of implemented unavailable methods from within the @implementation context .

LGTM, thanks!

Wed, Jan 9, 12:30 PM
erik.pilkington added inline comments to D56469: [ObjC] Allow the use of implemented unavailable methods from within the @implementation context .
Wed, Jan 9, 10:22 AM

Tue, Jan 8

erik.pilkington updated the diff for D56405: Split -Wdelete-non-virtual-dtor into two groups.

Split -Wdelete-non-virtual-dtor into -Wdelete-non-abstract-non-virtual-dtor and -Wdelete-abstract-non-virtual-dtor.

Tue, Jan 8, 10:06 AM
erik.pilkington planned changes to D56405: Split -Wdelete-non-virtual-dtor into two groups.
Tue, Jan 8, 9:37 AM
erik.pilkington reopened D56405: Split -Wdelete-non-virtual-dtor into two groups.

I reverted my commit in r350639.

Tue, Jan 8, 9:37 AM

Mon, Jan 7

erik.pilkington created D56405: Split -Wdelete-non-virtual-dtor into two groups.
Mon, Jan 7, 12:15 PM

Thu, Jan 3

erik.pilkington updated the diff for D55865: [ObjC] Add a new attribute to opt-out of implicit callee retain/release in ARC.

Mention __has_attribute and the ignored without -fobjc-arc thing. @aaron.ballman: Do you have any more thoughts here?

Thu, Jan 3, 8:57 PM
erik.pilkington updated the diff for D55865: [ObjC] Add a new attribute to opt-out of implicit callee retain/release in ARC.

Remove support for parameter indices.

Thu, Jan 3, 3:08 PM
erik.pilkington added inline comments to D55865: [ObjC] Add a new attribute to opt-out of implicit callee retain/release in ARC.
Thu, Jan 3, 1:52 PM
erik.pilkington updated the diff for D55865: [ObjC] Add a new attribute to opt-out of implicit callee retain/release in ARC.

Address John's comments. Thanks!

Thu, Jan 3, 1:33 PM

Wed, Jan 2

erik.pilkington added inline comments to D55865: [ObjC] Add a new attribute to opt-out of implicit callee retain/release in ARC.
Wed, Jan 2, 4:37 PM
erik.pilkington updated the diff for D55865: [ObjC] Add a new attribute to opt-out of implicit callee retain/release in ARC.

Address review comments. Also, make the attribute apply to parameters via the function declaration instead to parameters directly. i.e.:

__attribute__((objc_externally_retained(1)))
void foo(Widget *w) { ... }

The rule being that when this is applied to a non-param variable, it behaves as before, but when applied to a function it takes zero or more indices that correspond to which parameters are pseudo-strong. If no indices are specified, then it applies to every parameter that isn't explicitly __strong. This has a couple of advantages:

Wed, Jan 2, 4:36 PM
erik.pilkington accepted D55949: Correctly handle function pointers returning a type marked nodiscard.

Still LG, sorry for the delay!

Wed, Jan 2, 12:27 PM

Dec 20 2018

erik.pilkington updated the diff for D55865: [ObjC] Add a new attribute to opt-out of implicit callee retain/release in ARC.

Add some more Sema tests as per Aaron's comments.

Dec 20 2018, 4:19 PM
erik.pilkington added inline comments to D55865: [ObjC] Add a new attribute to opt-out of implicit callee retain/release in ARC.
Dec 20 2018, 4:19 PM
erik.pilkington accepted D55949: Correctly handle function pointers returning a type marked nodiscard.

LGTM!

Dec 20 2018, 2:13 PM
erik.pilkington added inline comments to D55865: [ObjC] Add a new attribute to opt-out of implicit callee retain/release in ARC.
Dec 20 2018, 1:06 PM
erik.pilkington updated the diff for D55865: [ObjC] Add a new attribute to opt-out of implicit callee retain/release in ARC.

Address some review comments. Thanks!

Dec 20 2018, 1:06 PM

Dec 19 2018

erik.pilkington added inline comments to D55628: Add support for "labels" on push/pop directives in #pragma clang attribute.
Dec 19 2018, 1:34 PM
erik.pilkington updated the diff for D55628: Add support for "labels" on push/pop directives in #pragma clang attribute.

Add Aaron's testcase, improve the documentation a bit.

Dec 19 2018, 1:34 PM
erik.pilkington added inline comments to D55865: [ObjC] Add a new attribute to opt-out of implicit callee retain/release in ARC.
Dec 19 2018, 11:34 AM
erik.pilkington updated the diff for D55865: [ObjC] Add a new attribute to opt-out of implicit callee retain/release in ARC.

Address @aaron.ballman comments, rebase onto r349535. Thanks!

Dec 19 2018, 11:34 AM

Dec 18 2018

erik.pilkington added a comment to D55628: Add support for "labels" on push/pop directives in #pragma clang attribute.

Just to be clear, this is just a syntactic change from what I originally posted. The old #pragma clang attribute push NoReturn (...) is semantically equivalent to the new #pragma clang attribute NoReturn.push (...)

Dec 18 2018, 11:45 PM
erik.pilkington updated the diff for D55628: Add support for "labels" on push/pop directives in #pragma clang attribute.

After looking through some users of #pragma clang attribute, I don't think that the begin/end solution is what we want here. Some users of this attribute write macros that can expand to different attributes depending on their arguments, for instance:

AVAILABILITY_BEGIN(macos(10.12)) // expands to an availability attribute
AVAILABILITY_BEGIN(ios(10))
// some code...
AVAILABILITY_END
AVAILABILITY_END

This code makes sense and is safe, but in this case rewriting AVAILABILITY_BEGIN to use a hypothetical pragma clang attribute begin/end would be a breaking change, which isn't acceptable. So I think that we want stack semantics, but scoped within the AVAILABILITY_BEGIN macro's namespace. That way, we can support multiple pushes in the same macro, without having having different macros accidentally pop each other.

Dec 18 2018, 11:36 PM
erik.pilkington created D55865: [ObjC] Add a new attribute to opt-out of implicit callee retain/release in ARC.
Dec 18 2018, 4:34 PM

Dec 14 2018

erik.pilkington added a comment to D54780: [llvm-demangle-fuzzer] Also fuzz microsoftDemangle()..
Dec 14 2018, 10:54 AM
erik.pilkington added inline comments to D55628: Add support for "labels" on push/pop directives in #pragma clang attribute.
Dec 14 2018, 10:42 AM
erik.pilkington updated the diff for D55628: Add support for "labels" on push/pop directives in #pragma clang attribute.

Address @aaron.ballman comments. Thanks!

Dec 14 2018, 10:42 AM
erik.pilkington added a comment to D55713: Let llvm-demangle-fuzzer fuzz microsoftDemangle as well.

Isn't this already being done in https://reviews.llvm.org/D54780? Have you coordinated with @morehouse?

Dec 14 2018, 10:23 AM
erik.pilkington added inline comments to D55628: Add support for "labels" on push/pop directives in #pragma clang attribute.
Dec 14 2018, 9:45 AM

Dec 12 2018

erik.pilkington created D55628: Add support for "labels" on push/pop directives in #pragma clang attribute.
Dec 12 2018, 4:28 PM

Dec 7 2018

erik.pilkington added a comment to D55097: [constexpr][c++2a] Try-catch blocks in constexpr functions.

LGTM, but you should probably let @rsmith have the final word!

Dec 7 2018, 4:45 PM

Dec 6 2018

erik.pilkington accepted D55095: [Preprocessor] Don't avoid entering included files after hitting a fatal error..

Nice, LGTM!

Dec 6 2018, 2:22 PM

Dec 4 2018

erik.pilkington added inline comments to D55095: [Preprocessor] Don't avoid entering included files after hitting a fatal error..
Dec 4 2018, 2:57 PM
erik.pilkington added a comment to D54780: [llvm-demangle-fuzzer] Also fuzz microsoftDemangle()..

Hi, thanks for working on this! Fuzzing in the itanium demangler has caught *a lot* of bugs, so this seems pretty useful. Whats the argument for testing the two demanglers in the same path? There isn't really any shared code, so I think it makes more sense to separate these out.

Dec 4 2018, 2:03 PM
erik.pilkington added a comment to D55097: [constexpr][c++2a] Try-catch blocks in constexpr functions.

Hi Bruno, thanks for working on this!

Dec 4 2018, 1:51 PM
erik.pilkington added a comment to D55233: Add objc_retain and objc_release intrinsics and codegen them to their runtime methods.

LGTM

Thanks!

Erik, did you have anything else required before landing this?

Dec 4 2018, 11:31 AM

Dec 3 2018

erik.pilkington added a comment to D55233: Add objc_retain and objc_release intrinsics and codegen them to their runtime methods.

Can you add these intrinsics to the LangRef? Seems pretty reasonable, but I don't feel comfortable LGTMing.

Dec 3 2018, 3:01 PM

Nov 29 2018

erik.pilkington added a comment to D55039: [sema] Warn of mangling change if function parameters are noexcept..

It seems like this check isn't really doing enough to catch this break in full generality. For instance, the mangling of the following changes from C++14 to 17, but isn't diagnosed here:

template <class T> struct something {};
void f(something<void (*)() noexcept>) {}
Nov 29 2018, 2:00 PM

Nov 26 2018

erik.pilkington accepted D54893: [Demangle] remove itaniumFindTypesInMangledName.
Nov 26 2018, 8:57 AM

Nov 11 2018

erik.pilkington created D54414: [Sema] Make sure we substitute an instantiation-dependent default template parameter.
Nov 11 2018, 10:33 PM
erik.pilkington added a comment to D54344: [Clang][CodeGen][CXX]: Workaround __attribute((no_destroy)) crash/incorrect code generation..

LGTM too, with one small fix in test. Thanks for working on this!

Well, I figured since the assertion being tripped was (before investigation) the only reliable way to notice the bug it makes sense for it to stay in, main concern being that should anything regress and assertions are off, the code generation is essentially undefined. Though a good argument for taking it out would be the fact that currently it's the only test that verifies IR generated with the attribute (last time I checked), but I would also imagine most people running tests would have assertion enabled (or debug) builds.

Nov 11 2018, 5:11 PM
erik.pilkington accepted D54344: [Clang][CodeGen][CXX]: Workaround __attribute((no_destroy)) crash/incorrect code generation..

LGTM too, with one small fix in test. Thanks for working on this!

Nov 11 2018, 4:40 PM

Nov 10 2018

erik.pilkington added a comment to D54344: [Clang][CodeGen][CXX]: Workaround __attribute((no_destroy)) crash/incorrect code generation..

I have a few nits, but I think this looks about right.

Nov 10 2018, 11:53 AM

Nov 9 2018

erik.pilkington added inline comments to D53522: [Frontend] Include module map header declaration in dependency file output.
Nov 9 2018, 3:29 PM
erik.pilkington added a reviewer for D54344: [Clang][CodeGen][CXX]: Workaround __attribute((no_destroy)) crash/incorrect code generation.: erik.pilkington.

Have you tried running creduce on the preprocessed source? We should really have a real reproducer for this, otherwise its really hard to tell what the underlying problem is.

Nov 9 2018, 12:27 PM

Nov 5 2018

erik.pilkington added a comment to D53522: [Frontend] Include module map header declaration in dependency file output.

Ping!

Nov 5 2018, 2:27 PM
erik.pilkington added a comment to D54074: CPlusPlusLanguage: Use new demangler API to implement type substitution.

Looks good to me too, thanks!

Nov 5 2018, 11:00 AM

Nov 2 2018

erik.pilkington added a comment to D54055: CGDecl::emitStoresForConstant fix synthesized constant's name.

Can you give an example of some code that triggered this with your patch applied? Even if it isn't a real reproducer right now, it would help to try to understand whats happening here.

Nov 2 2018, 4:19 PM

Nov 1 2018

erik.pilkington updated the diff for D54010: [CodeGen] Fix a crash when updating a zeroinitialize designated initializer.

Use getAggregateElement. Thanks!

Nov 1 2018, 10:59 PM
erik.pilkington added inline comments to D54010: [CodeGen] Fix a crash when updating a zeroinitialize designated initializer.
Nov 1 2018, 10:59 PM
erik.pilkington created D54010: [CodeGen] Fix a crash when updating a zeroinitialize designated initializer.
Nov 1 2018, 4:32 PM

Oct 31 2018

erik.pilkington added a comment to D48896: [libcxx][c++17] P0083R5: Splicing Maps and Sets Part 2: merge.

Thanks for reviewing!

Oct 31 2018, 10:34 AM

Oct 30 2018

erik.pilkington added a comment to D48896: [libcxx][c++17] P0083R5: Splicing Maps and Sets Part 2: merge.

We can only do that if the comparators divide elements into the same equivalence classes, which we cannot know in general. (We can do this optimization if we can prove the comparator is a pure stateless function, but we probably can't detect that -- even an empty class type could store its state outside the class.) However, if the source and destination have the same comparator, and that comparator is std::less<T>, then I think it's correct to do this optimization.

Hmm, actually, we can probably do this in more cases than that. If we use the *destination's* comparator to check for equivalent elements instead of the source's comparator (under the assumption that the destination order is usually the same as the source order), this would work.

Using the destination's comparator to perform this optimization is clever, and I believe it would work. This is much better than using the source's comparator when std::is_empty<SourceComparator> and std::is_same<SourceComparator, DestinationComparator> are both true.

But more generally, can we just provide the location of the most-recently-inserted element as an insertion hint and get this optimization as a special case of a more general optimization? But maybe that's actually what you were suggesting (the difference is hidden behind the "key in the source is not equivalent to the [other] key" comments -- equivalent according to which comparator?).

The insertion hint idea did cross my mind when I was thinking about this but I did not pursue it further. I think it may be a cleaner way to achieve what I'm suggesting.

Oct 30 2018, 11:30 PM
erik.pilkington updated the diff for D48896: [libcxx][c++17] P0083R5: Splicing Maps and Sets Part 2: merge.

Address review comments. Thanks!

Oct 30 2018, 11:30 PM
erik.pilkington added inline comments to D53547: NFC: Remove the ObjC1/ObjC2 distinction from clang (and related projects).
Oct 30 2018, 1:55 PM
erik.pilkington added inline comments to D53522: [Frontend] Include module map header declaration in dependency file output.
Oct 30 2018, 9:30 AM
erik.pilkington updated the diff for D53522: [Frontend] Include module map header declaration in dependency file output.

Address review comments. Thanks!

Oct 30 2018, 9:30 AM

Oct 29 2018

erik.pilkington added inline comments to D53538: NFC: Reorganize the demangler a bit.
Oct 29 2018, 11:21 AM
erik.pilkington updated the diff for D53538: NFC: Reorganize the demangler a bit.

Use an impersonal tone in the README, remove DEMANGLE_ENABLE_DUMP.

Oct 29 2018, 11:21 AM

Oct 28 2018

erik.pilkington added inline comments to D53621: Support for groups of attributes in #pragma clang attribute.
Oct 28 2018, 7:30 PM

Oct 25 2018

erik.pilkington updated the diff for D53621: Support for groups of attributes in #pragma clang attribute.

Add documentation and release notes, {}s. Thanks!

Oct 25 2018, 4:22 PM

Oct 24 2018

erik.pilkington added a comment to D53678: Include llvm-config.h from Demangle/Compiler.h.

FYI: this will be fixed by https://reviews.llvm.org/D53538 when that lands.

Oct 24 2018, 3:43 PM

Oct 23 2018

erik.pilkington created D53621: Support for groups of attributes in #pragma clang attribute.
Oct 23 2018, 4:02 PM
erik.pilkington updated the diff for D53538: NFC: Reorganize the demangler a bit.
  1. Should we document in the README.txt that we want to split "demangle" out to a separate top-level directory some time after the monorepo move?
Oct 23 2018, 1:14 PM
erik.pilkington added inline comments to D53595: [C++17] Reject shadowing of capture by parameter in lambda.
Oct 23 2018, 12:38 PM
erik.pilkington added a comment to D53595: [C++17] Reject shadowing of capture by parameter in lambda.

Thanks for working on this! Can you update www/cxx_dr_status.html too?

Oct 23 2018, 12:28 PM
erik.pilkington updated subscribers of D53547: NFC: Remove the ObjC1/ObjC2 distinction from clang (and related projects).
Oct 23 2018, 11:09 AM
erik.pilkington updated the diff for D53547: NFC: Remove the ObjC1/ObjC2 distinction from clang (and related projects).

Fix some spacing mistakes. Thanks!

Oct 23 2018, 11:09 AM

Oct 22 2018

erik.pilkington added a comment to D48896: [libcxx][c++17] P0083R5: Splicing Maps and Sets Part 2: merge.

Pong!

Oct 22 2018, 6:23 PM
erik.pilkington created D53547: NFC: Remove the ObjC1/ObjC2 distinction from clang (and related projects).
Oct 22 2018, 6:17 PM
erik.pilkington created D53538: NFC: Reorganize the demangler a bit.
Oct 22 2018, 4:44 PM
erik.pilkington created D53522: [Frontend] Include module map header declaration in dependency file output.
Oct 22 2018, 12:47 PM