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 (225 w, 5 d)

Recent Activity

Wed, Jul 11

dexonsmith closed D48723: Fix IRPrinting bug.

Committed in r336869!

Wed, Jul 11, 4:56 PM
dexonsmith committed rL336869: IR: Skip -print-*-all after -print-*.
IR: Skip -print-*-all after -print-*
Wed, Jul 11, 4:35 PM

Tue, Jul 10

dexonsmith added a dependency for D49163: ADT: Shrink SmallVector size 0 to 16B on 64-bit platforms: D48518: ADT: Shrink SmallVector by 8B on 64-bit platforms.
Tue, Jul 10, 5:15 PM
dexonsmith added a dependent revision for D48518: ADT: Shrink SmallVector by 8B on 64-bit platforms: D49163: ADT: Shrink SmallVector size 0 to 16B on 64-bit platforms.
Tue, Jul 10, 5:15 PM
dexonsmith created D49163: ADT: Shrink SmallVector size 0 to 16B on 64-bit platforms.
Tue, Jul 10, 5:14 PM
dexonsmith added a comment to D48518: ADT: Shrink SmallVector by 8B on 64-bit platforms.

Ping!

Tue, Jul 10, 4:24 PM
dexonsmith added a comment to D48991: Docs: Spell C++ lambdas like functions, not variables.

Only lambdas, and not other functors (like std::function, etc)?

So we could end up with code like:

MyFunctor = myLambda;

Though I suppose that's OK - and it's only in the local scope of a function
& not like any interesting object-like operations (such as assignment) can
be done with the lambda as the subject anyway, so it does look a lot like a
function... (except you can't assign it to a function pointer - but at
least the lambda should be nearby in the code so that's not too confusing)

Tue, Jul 10, 4:23 PM
dexonsmith added a comment to D49138: [LTO] Handle __imp_ (dllimport) symbols consistently with lld.

Thanks! (I'm not comfortable signing off though, since I don't know much about dllimport.)

Tue, Jul 10, 9:36 AM
dexonsmith added a comment to D49138: [LTO] Handle __imp_ (dllimport) symbols consistently with lld.

Is it impossible to also test this locally (in the LLVM repo)? It will be hard to hack on this file if there aren't local tests.

Tue, Jul 10, 9:03 AM
dexonsmith accepted D49090: [ThinLTO] Escape module paths when printing.

LGTM too.

Tue, Jul 10, 9:00 AM

Mon, Jul 9

dexonsmith added inline comments to D46274: [Support] Harden JSON against invalid UTF-8..
Mon, Jul 9, 11:51 AM
dexonsmith requested changes to D49090: [ThinLTO] Escape module paths when printing.
Mon, Jul 9, 11:25 AM
dexonsmith accepted D49051: [libclang] check that the cursor is declaration before trying to evaluate the cursor like a declaration.

LGTM.

Mon, Jul 9, 8:25 AM

Sat, Jul 7

dexonsmith added a comment to D49030: Add OrderedSet, with constant-time insertion and removal, and random access iteration..

We already have SetVector which is widely used for these patterns. If we need both, we need a clear explanation of the difference and why we need both (IE, why users of one can't use the other).

The remove operation on a SetVector can cost linear time and this might be an issue in some cases. Please have a look at the review comments of https://reviews.llvm.org/D48372

Sat, Jul 7, 7:56 AM

Thu, Jul 5

dexonsmith added a comment to D48857: Also search BitcodeFiles for exclude-lib symbols.

It may make sense to get a consensus and update the coding standard
document, as it is not entirely clear. If that's the case, I'd vote for
CamelCase. :)

Thu, Jul 5, 1:00 PM
dexonsmith added a comment to D48991: Docs: Spell C++ lambdas like functions, not variables.

I've started an RFC on llvm-dev to discuss whether we should make this change:
http://lists.llvm.org/pipermail/llvm-dev/2018-July/124466.html

Thu, Jul 5, 12:58 PM
dexonsmith created D48991: Docs: Spell C++ lambdas like functions, not variables.
Thu, Jul 5, 12:54 PM
dexonsmith added a comment to D48857: Also search BitcodeFiles for exclude-lib symbols.

Is that part of our coding standard? In lld, all local variables holding
function-like objects are in CamelCase, not in camelCase.

Thu, Jul 5, 12:46 PM
dexonsmith accepted D48723: Fix IRPrinting bug.

Anyway, if this is not the right way to do, then we should change BitcodeWriter to a non-analysis pass I guess? There should be no difference between these 2 passes on whether it is an analysis or not IMHO

Thu, Jul 5, 12:13 PM
dexonsmith added inline comments to D48857: Also search BitcodeFiles for exclude-lib symbols.
Thu, Jul 5, 12:04 PM
dexonsmith added a comment to D48892: [libc++] Replace uses of _LIBCPP_ALWAYS_INLINE by _LIBCPP_INLINE_VISIBILITY.

The lldb bot started failing very recently and the blamelist hints at this change.

http://green.lab.llvm.org/green/job/lldb-cmake/7777/

Can you please take a look?

For your convenience, this is failing building LibCxx testcases with a linker error:

Build Command Output:
Undefined symbols for architecture x86_64:
  "std::__1::__vector_base_common<true>::__vector_base_common()", referenced from:
      std::__1::__vector_base<int, std::__1::allocator<int> >::__vector_base() in main.o
      std::__1::__vector_base<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >, std::__1::allocator<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > > >::__vector_base() in main.o
ld: symbol(s) not found for architecture x86_64
clang-7: error: linker command failed with exit code 1 (use -v to see invocation)
make: *** [a.out] Error 1
Thu, Jul 5, 11:37 AM
dexonsmith added a comment to D48983: [libc++] Replace incorrect visibility on a streambuf method.

It's pretty obvious that this is the problem when looking at https://reviews.llvm.org/D48892 (grep for __pbump), but TBH I haven't been able to run the check-cxx-abilist test locally. Mine fails with like 500 differences, so there's got to be something I'm doing wrong.

Thu, Jul 5, 11:30 AM
dexonsmith accepted D48983: [libc++] Replace incorrect visibility on a streambuf method.

LGTM.

Thu, Jul 5, 11:29 AM
dexonsmith accepted D48939: CallGraph passes: iterate over all functions rather than just externally visible ones.

LGTM.

Thu, Jul 5, 11:26 AM

Wed, Jul 4

dexonsmith added a reviewer for D48939: CallGraph passes: iterate over all functions rather than just externally visible ones: dexonsmith.

Awesome. I wonder, would it be worth measuring compile-time impact for optimized builds?

Wed, Jul 4, 10:04 AM

Tue, Jul 3

dexonsmith added inline comments to D48892: [libc++] Replace uses of _LIBCPP_ALWAYS_INLINE by _LIBCPP_INLINE_VISIBILITY.
Tue, Jul 3, 12:57 PM
dexonsmith added a reviewer for D48892: [libc++] Replace uses of _LIBCPP_ALWAYS_INLINE by _LIBCPP_INLINE_VISIBILITY: mclow.lists.

FWIW, this change LGTM... but I wouldn't mind hearing from Eric or Marshall before commit.

Tue, Jul 3, 12:50 PM

Mon, Jul 2

dexonsmith added a comment to D48698: [ThinLTO] Fix printing of module paths for distributed backend indexes.

What was the numbering before?

Mon, Jul 2, 10:50 AM

Fri, Jun 29

dexonsmith added a comment to D48723: Fix IRPrinting bug.

No don't get me wrong, this patch is supposed to fix the issue in the LegacyPassManager, and I do not intend to change anything about the new pass manager in this patch.

As to implementing print-after-all in the new pass manager, I do not know what should be the best way to do yet... Your idea seems to be a good solution.

Fri, Jun 29, 10:18 AM

Thu, Jun 28

dexonsmith resigned from D34249: [libc++] Don't use UTIME_OMIT to detect utimensat on Apple.
Thu, Jun 28, 8:56 PM
dexonsmith added a comment to D48723: Fix IRPrinting bug.

I'm not sure this technique will work in the new pass manager, since analyses are run on-demand IIRC. I wonder if it would be better to leave these as passes, and implement -print-after-all by inserting -print-module/etc. passes into the pipeline as the pipeline is being constructed. You could avoid adding extra -print-module statements in cases where that pass already exists.

Thu, Jun 28, 5:51 PM
dexonsmith requested changes to D48723: Fix IRPrinting bug.

This seems like a reasonable thing to do. Please add tests to check the output of both pass managers.

Thu, Jun 28, 9:02 AM
dexonsmith added inline comments to D48693: [WebAssembly] LTO: Fix signatures of bitcode symbols.
Thu, Jun 28, 8:47 AM

Wed, Jun 27

dexonsmith added a reviewer for D48694: [libc++abi] Limit libc++ header search to specified paths: ldionne.

What's the effective change on the command-line? Does this map to -nostdinc? To -nostdinc++?

Wed, Jun 27, 5:48 PM
dexonsmith updated subscribers of D48675: [libc++abi] Limit libc++ header search to specified paths.

Please close this and open a new one. Adding cfe-commits after the fact will fail to send the patch/description to cfe-commits, and not everyone is on llvm-commits.

Wed, Jun 27, 5:40 PM
dexonsmith resigned from D48369: [CodeGen] Make block removal order deterministic in CodeGenPrepare.

Resigning as reviewer (thanks for fixing the complexity); I'll let the other reviewers sort out if SmallVector is an even better solution here.

Wed, Jun 27, 8:52 AM

Tue, Jun 26

dexonsmith added inline comments to D48241: [DebugInfo] Emit ObjC methods as part of interface..
Tue, Jun 26, 9:44 AM
dexonsmith accepted D47906: [ThinLTO] Add testing of summary index parsing to a couple CFI tests.

LGTM.

Tue, Jun 26, 8:29 AM

Mon, Jun 25

dexonsmith added a comment to D47842: [ThinLTO] Add string saver onto index for value names.
In D47842#1142966, @pcc wrote:
In D47842#1142784, @pcc wrote:

What is the goal of making Saver and Alloc optional? Is it to reduce the size of the structure? If that's the case, it doesn't look like it will help, in fact it will increase the size because sizeof(Optional<T>) > sizeof(T).
http://llvm-cs.pcc.me.uk/include/llvm/ADT/Optional.h#114

My recollection is that BumpPtrAllocator does its first heap allocation on construction. The point was to avoid that allocation when it was unnecessary. Have I remembered incorrectly?

It doesn't appear to do that. BumpPtrAllocator's default constructor (http://llvm-cs.pcc.me.uk/include/llvm/Support/Allocator.h#152) uses = default;.

Mon, Jun 25, 3:52 PM
dexonsmith accepted D47844: [ThinLTO] Compute GUID directly from GV when building per-module index.

Okay, this LGTM then.

Mon, Jun 25, 3:50 PM
dexonsmith added a comment to D47842: [ThinLTO] Add string saver onto index for value names.
In D47842#1142784, @pcc wrote:

What is the goal of making Saver and Alloc optional? Is it to reduce the size of the structure? If that's the case, it doesn't look like it will help, in fact it will increase the size because sizeof(Optional<T>) > sizeof(T).
http://llvm-cs.pcc.me.uk/include/llvm/ADT/Optional.h#114

Mon, Jun 25, 3:44 PM
dexonsmith added a comment to D47844: [ThinLTO] Compute GUID directly from GV when building per-module index.

What's the potential overhead of this choice (memory, runtime)?

Mon, Jun 25, 1:58 PM
dexonsmith accepted D47842: [ThinLTO] Add string saver onto index for value names.

LGTM.

Mon, Jun 25, 1:54 PM

Sat, Jun 23

dexonsmith updated the diff for D48518: ADT: Shrink SmallVector by 8B on 64-bit platforms.

Updating the patch since I decided to just lift set_size() up to SmallVectorBase. My ASan-ified check-llvm and check-clang came back happy.

Sat, Jun 23, 4:11 PM
dexonsmith closed D48516: ADT: Use EBO to shrink SmallVector size 1.

Committed in r335421.

Sat, Jun 23, 11:49 AM
dexonsmith committed rL335421: ADT: Use EBO to shrink SmallVector size 1.
ADT: Use EBO to shrink SmallVector size 1
Sat, Jun 23, 11:44 AM
dexonsmith added a dependency for D48518: ADT: Shrink SmallVector by 8B on 64-bit platforms: D48516: ADT: Use EBO to shrink SmallVector size 1.
Sat, Jun 23, 8:40 AM
dexonsmith added a dependent revision for D48516: ADT: Use EBO to shrink SmallVector size 1: D48518: ADT: Shrink SmallVector by 8B on 64-bit platforms.
Sat, Jun 23, 8:40 AM
dexonsmith created D48518: ADT: Shrink SmallVector by 8B on 64-bit platforms.
Sat, Jun 23, 8:40 AM

Fri, Jun 22

dexonsmith created D48516: ADT: Use EBO to shrink SmallVector size 1.
Fri, Jun 22, 8:54 PM
dexonsmith accepted D47905: [ThinLTO] Parse module summary index from assembly.

I still think you might be able to use an Optional (see my inline comment) but I don't need to see this again.

Fri, Jun 22, 7:46 PM

Thu, Jun 21

dexonsmith requested changes to D48369: [CodeGen] Make block removal order deterministic in CodeGenPrepare.
Thu, Jun 21, 8:41 AM

Wed, Jun 20

dexonsmith requested changes to D47905: [ThinLTO] Parse module summary index from assembly.

I just have a couple of additional (minor) comments on the code, although I admit it was hard to be thorough as a it's a monolithic patch. I might have preferred if this were committed incrementally, adding support for 1-2 constructs at a time. Because the lexing/parsing code is fairly mechanical, I think splitting it up now would be quite likely to introduce errors so I'm not going to ask for that.

Wed, Jun 20, 9:38 AM

Tue, Jun 19

dexonsmith added a comment to D47905: [ThinLTO] Parse module summary index from assembly.

I've just had time to skim a few files; feel free to ping me privately if I don't continue the review later today.

Tue, Jun 19, 9:25 AM

Mon, Jun 18

dexonsmith added a comment to D47687: [Sema] Missing -Wlogical-op-parentheses warnings in macros (PR18971).

Did you miss this comment from my previous review?

Mon, Jun 18, 8:47 AM

Sun, Jun 17

dexonsmith added inline comments to D47687: [Sema] Missing -Wlogical-op-parentheses warnings in macros (PR18971).
Sun, Jun 17, 12:18 PM

Sat, Jun 16

dexonsmith requested changes to D47687: [Sema] Missing -Wlogical-op-parentheses warnings in macros (PR18971).
Sat, Jun 16, 6:54 PM

Jun 15 2018

dexonsmith added reviewers for D47687: [Sema] Missing -Wlogical-op-parentheses warnings in macros (PR18971): arphaman, ahatanak.

I think I am almost there.

Here, In my sight

#define foo(op0, op1, x, y, z) ( (void)(x op0 y op1 z) )

is un-actionable, because x op0 y op1 z are from different arguments of function-like macro, so, we will not check parentheses for op0 or op1 when we call it by

foo(&&, ||, x, y, z)

but if we call it by

foo(&&, ||, x && y ||z, y, z)

it is actionable, because argument (x && y || z) is from the same arg position of macro foo;
hence we should check x && y || z but shouldn't check parentheses for the first argument (&&) and second argument (||)

Jun 15 2018, 10:27 AM

Jun 12 2018

dexonsmith updated subscribers of D47969: [Verifier] Check that ValueAsMetadata belongs to the module.

IIRC, sharing uniqued metadata across modules should only happen as a temporary state. It's a trick for when loading modules that will be merged together, to avoid unnecessarily creating multiple copies of the graphs.

Jun 12 2018, 8:44 AM

Jun 11 2018

dexonsmith added a comment to D47687: [Sema] Missing -Wlogical-op-parentheses warnings in macros (PR18971).
In D47687#1125926, @rnk wrote:

@dexonsmith is there someone from Apple who can comment on rdar://8678458 and the merits of disabling this warning in macros? I strongly suspect the original report was dealing with code like assert(x || y && "str");, if so we can go forward with this.

@chandlerc I know you've hit this behavior difference vs. GCC before. Any thoughts on the proposed change?

In D47687#1125926, @rnk wrote:

@dexonsmith is there someone from Apple who can comment on rdar://8678458 and the merits of disabling this warning in macros? I strongly suspect the original report was dealing with code like assert(x || y && "str");, if so we can go forward with this.

There were two commits with this radar: r119537 and r119540. The main motivation was a deeply nested macro that when "inlined" included the moral equivalent of #define DOUBLE_OP(OP1, OP2, X, Y, Z) (X OP1 Y OP2 Z). There was terrible note spew when the warning fired in this case, and the use case for the macro made the warning un-actionable. We decided to suppress the warning entirely:

As a general comment, the warning seems to be useless for macros; I'll follow the example of warn_logical_instead_of_bitwise which doesn't trigger for macros and I'll make the warning not warn for macros.

Hi, Thank you,

I noticed that warn_logical_instead_of_bitwise will also skip parentheses checking in macros... well, this patch seems not so necessary... both ok for me ... depends on all of you :-)

At worst, we can issue this warning in a new -Wparentheses-in-macros subgroup, which, if apple so insists, could be off-by-default.
That would less worse than just completely silencing it for the entire world.

I’d be fine with strengthening the existing warning as long as there is an actionable fix-it. I suspect if you suppress it when the relevant expression is constructed from multiple macro arguments that will be good enough.

Thanks, currently, [-Wparentheses | -Wlogical-op-parentheses] will not emit warning for parentheses in macros. only if you add [-Wlogical-op-parentheses-in-macros] it will emit something like '&&' within '||' warning...

However, '&' within '|' checking was disabled in macros as well... I don't know if this patch meet the needs... if this patch was ok, then, just as @lebedev.ri said, Maybe we could add a [-Wparentheses-in-macros] subgroup and add these warning into this new group, in the future... depends on users :-) any suggestion?

Jun 11 2018, 12:43 PM

Jun 8 2018

dexonsmith added a comment to D47687: [Sema] Missing -Wlogical-op-parentheses warnings in macros (PR18971).
In D47687#1125926, @rnk wrote:

@dexonsmith is there someone from Apple who can comment on rdar://8678458 and the merits of disabling this warning in macros? I strongly suspect the original report was dealing with code like assert(x || y && "str");, if so we can go forward with this.

@chandlerc I know you've hit this behavior difference vs. GCC before. Any thoughts on the proposed change?

In D47687#1125926, @rnk wrote:

@dexonsmith is there someone from Apple who can comment on rdar://8678458 and the merits of disabling this warning in macros? I strongly suspect the original report was dealing with code like assert(x || y && "str");, if so we can go forward with this.

There were two commits with this radar: r119537 and r119540. The main motivation was a deeply nested macro that when "inlined" included the moral equivalent of #define DOUBLE_OP(OP1, OP2, X, Y, Z) (X OP1 Y OP2 Z). There was terrible note spew when the warning fired in this case, and the use case for the macro made the warning un-actionable. We decided to suppress the warning entirely:

As a general comment, the warning seems to be useless for macros; I'll follow the example of warn_logical_instead_of_bitwise which doesn't trigger for macros and I'll make the warning not warn for macros.

Hi, Thank you,

I noticed that warn_logical_instead_of_bitwise will also skip parentheses checking in macros... well, this patch seems not so necessary... both ok for me ... depends on all of you :-)

At worst, we can issue this warning in a new -Wparentheses-in-macros subgroup, which, if apple so insists, could be off-by-default.
That would less worse than just completely silencing it for the entire world.

Jun 8 2018, 9:25 AM

Jun 7 2018

dexonsmith added a comment to D47687: [Sema] Missing -Wlogical-op-parentheses warnings in macros (PR18971).
In D47687#1125926, @rnk wrote:

@dexonsmith is there someone from Apple who can comment on rdar://8678458 and the merits of disabling this warning in macros? I strongly suspect the original report was dealing with code like assert(x || y && "str");, if so we can go forward with this.

Jun 7 2018, 6:42 PM

May 31 2018

dexonsmith added inline comments to D47157: Warning for framework headers using double quote includes.
May 31 2018, 12:40 PM
dexonsmith added inline comments to D47157: Warning for framework headers using double quote includes.
May 31 2018, 11:32 AM

May 30 2018

dexonsmith added inline comments to D47157: Warning for framework headers using double quote includes.
May 30 2018, 3:23 PM
dexonsmith added inline comments to D47157: Warning for framework headers using double quote includes.
May 30 2018, 3:12 PM

May 29 2018

dexonsmith added inline comments to D47157: Warning for framework headers using double quote includes.
May 29 2018, 3:38 PM

May 24 2018

dexonsmith accepted D46699: [ThinLTO] Print module summary index to assembly.

I have a suggestion for how to respond to Peter's feedback, but this LGTM once he's happy.

May 24 2018, 3:24 PM

May 23 2018

dexonsmith added a comment to D46699: [ThinLTO] Print module summary index to assembly.

The LangRef looks great, thanks for adding it. I have a few nitpicks and a follow-up on the SkipLineComment discussion. I made it most of the way through the AssemblyWriter (and any comments/questions there might apply more generally to the rest)... you may as well respond to those first, and then I can review the rest.

May 23 2018, 9:31 AM
dexonsmith added a comment to D47179: [LLVM-C] Add Bindings For Named Metadata.

Adding Duncan to the review list who (presumably) added the TODO and may have some thoughts on this.

Making NamedMDNode inherit from Metadata will allow MDNode to reference NamedMDNode.

  • What's the use case for this?
  • What complexity does it add to the IR? (Especially, during serialization and LTO...)

    Perhaps this is worth doing, but we need a compelling reason. In retrospect, I'm not sure I had sufficient motiviation when I added the TODO.
May 23 2018, 8:29 AM

May 22 2018

dexonsmith added a comment to D47179: [LLVM-C] Add Bindings For Named Metadata.

Adding Duncan to the review list who (presumably) added the TODO and may have some thoughts on this.

May 22 2018, 1:01 PM

May 21 2018

dexonsmith added a comment to D46485: Add python tool to dump and construct header maps.

Removing the binary header map files is a clear win, but I'd like someone else to approve this.

May 21 2018, 4:16 PM

May 17 2018

dexonsmith added inline comments to D46485: Add python tool to dump and construct header maps.
May 17 2018, 9:30 AM

May 16 2018

dexonsmith added a comment to D46834: [Sema][Cxx17] Error message for C++17 static_assert(pred) without string literal.

The policy certainly seems designed around the CLI use case. For serialized diagnostics, it would make sense to either serialize the snippet or enough information that the snippet can be reconstructed. And if that can't be done, or fails to satisfy some other use case, then we should discuss how we proceed -- for instance, we could consider having different diagnostic messages for the case where we have a snippet and for the case where we do not.

Right. There are places in the IDE where there is a condensed view of all diagnostics (like a Vim location list), and others where the diagnostics are shown inline with the sources. I think what we want is an optional auxiliary record/field in a diagnostic with that contains context for when the source context is missing, and then the IDE can choose which to display. It's optional because most diagnostics are good enough as is for location lists.

That sounds good to me. I think it would also make sense to use the alternate form for the CLI case if the user is using -fno-caret-diagnostics for some reason.

May 16 2018, 6:34 PM · Restricted Project
dexonsmith added a comment to D46834: [Sema][Cxx17] Error message for C++17 static_assert(pred) without string literal.

We reconsidered this in light of the policy - thanks for pointing that out Richard!
Just to be sure that I understand it - the policy is meant for CLI and not serialized diagnostics, right?

The policy certainly seems designed around the CLI use case. For serialized diagnostics, it would make sense to either serialize the snippet or enough information that the snippet can be reconstructed. And if that can't be done, or fails to satisfy some other use case, then we should discuss how we proceed -- for instance, we could consider having different diagnostic messages for the case where we have a snippet and for the case where we do not.

May 16 2018, 6:28 PM · Restricted Project
dexonsmith accepted D46858: Signal handling should be signal-safe.

I found a couple more nits to pick while reading the new version of the lock-free code. LGTM once you fix those.

May 16 2018, 10:02 AM
dexonsmith added inline comments to D46699: [ThinLTO] Print module summary index to assembly.
May 16 2018, 8:18 AM

May 15 2018

dexonsmith requested changes to D46699: [ThinLTO] Print module summary index to assembly.

I haven't had time to give this a full review, but skimming this I noticed that there's no update to llvm/docs/LangRef.rst. Can you document the syntax and semantics there?

May 15 2018, 8:59 PM
dexonsmith added a comment to D40864: [Darwin] Add a new -mstack-probe option and enable by default.

Did this eventually go in?

May 15 2018, 6:12 PM
dexonsmith resigned from D38666: Add constructor with cl::Option argument for FilteredPassNameParser.
May 15 2018, 12:45 PM
dexonsmith added a reviewer for D17741: adds __FILE_BASENAME__ builtin macro: vsapsai.

split the original into two parts. This one supports -ffile-macro-prefix-to-remove function.

I locally verified it. To add a test case, do we need to use VFS?

May 15 2018, 12:41 PM
dexonsmith resigned from D41697: [DebugInfo][Metadata] Add support for a DIExpression as 'count' field of DISubrange..
May 15 2018, 12:39 PM
dexonsmith added a comment to D46834: [Sema][Cxx17] Error message for C++17 static_assert(pred) without string literal.

(Somehow I missed Richard's comment when I replied last night, even though it preceded mine by 5 hours...)

May 15 2018, 8:39 AM · Restricted Project

May 14 2018

dexonsmith added a comment to D46858: Signal handling should be signal-safe.
In D46858#1098875, @jfb wrote:

I committed the two requests NFC changes. Will address other changes tomorrow.

May 14 2018, 9:37 PM
dexonsmith accepted D46858: Signal handling should be signal-safe.

Since you've answered my question about testing, this LGTM after you address the other feedback. Thanks for splitting out the two commits.

May 14 2018, 9:37 PM
dexonsmith requested changes to D46834: [Sema][Cxx17] Error message for C++17 static_assert(pred) without string literal.

The code looks correct to me, although I have a few suggestions inline. When you resubmit, please include full context (e.g., git diff -U999999).

May 14 2018, 9:31 PM · Restricted Project
dexonsmith added a comment to D46858: Signal handling should be signal-safe.
In D46858#1098865, @jfb wrote:

(Will address code changes separately)

I wonder if there's some way to write a test for this. E.g., a unit test that registers a signal handler for SIGUSR1 and then does what used to be necessary to trigger a crash.

Hmm I don't think so, at least not a reliable test: most of the time the test wouldn't hit a dangerous interleaving. We could try to make the bad concurrent interleaving more likely, or loop a bunch so it's more likely to hit the race because it tries more, but I think fundamentally it would be a flaky "success" test, and mostly a test that does nothing. Were the code to start being broken, it would become a flaky failure.

May 14 2018, 9:15 PM
dexonsmith requested changes to D46858: Signal handling should be signal-safe.

I wonder if there's some way to write a test for this. E.g., a unit test that registers a signal handler for SIGUSR1 and then does what used to be necessary to trigger a crash.

May 14 2018, 6:10 PM

May 11 2018

dexonsmith edited reviewers for D46778: Remove the fixit for the diagnostics regarding capturing autoreleasing variables in a block, added: ahatanak; removed: dexonsmith.
May 11 2018, 4:41 PM

May 10 2018

dexonsmith added a comment to D46694: [diagtool] Install diagtool.

This SGTM, but I wouldn't mind hearing from others. I wonder if this is worth a quick RFC on cfe-dev?

May 10 2018, 9:30 AM
dexonsmith added inline comments to D45930: [Support] Upstream anonymization and manipulating of BCSymbolMaps.
May 10 2018, 9:20 AM
dexonsmith added inline comments to D45930: [Support] Upstream anonymization and manipulating of BCSymbolMaps.
May 10 2018, 8:38 AM

Apr 30 2018

dexonsmith added inline comments to D46271: [CodeView] Improve debbuging of virtual base class member variables.
Apr 30 2018, 1:04 PM · debug-info

Apr 23 2018

dexonsmith added a comment to D45772: [Minor patch] Fix IR Module Printing.

Another high-level note (besides adding tests): please resubmit the patch with full context (e.g., git diff -U9999999 HEAD^..).

Apr 23 2018, 11:47 AM
dexonsmith resigned from D45975: [DebugInfo] Invalidate debug info in ReassociatePass::RewriteExprTree.
Apr 23 2018, 11:15 AM · debug-info
dexonsmith added a comment to D30882: Add a callback for __has_include and use it for dependency scanning.

Would it be ok to turn this on by default, without a flag, only in the case of the path actually existing, and only the found path being the one we add to the .d?

I think that pessimizes some incremental builds:

  • You have a __has_include("missing.h"), but don't include missing.h.
  • Change "missing.h" (but don't delete it).
  • An incremental build now has to rebuild the object file, even though nothing will have changed.

    However, it's fixing an actual bug, so it makes sense to me to be more conservative.
Apr 23 2018, 11:11 AM
dexonsmith added a comment to D30882: Add a callback for __has_include and use it for dependency scanning.

Oh, that actually wasn't my intention when I wrote it.

Honestly I didn't expect it to log anything for missing paths at all, as we don't currently log all the missing (but attempted) paths for regular #include's.

Apr 23 2018, 9:55 AM
dexonsmith added a comment to D30882: Add a callback for __has_include and use it for dependency scanning.

I don't think this is quite right. I know at least make-based incremental builds wouldn't deal well with this.

This is actually not a novel problem w.r.t. this patch. The exact same situation comes up with Makefile-included .d files and when one of the referenced headers is removed.

This is typically solved somewhere in the build system, for example Make has -include, and Ninja and llbuild have explicit support for this situation.

Apr 23 2018, 8:27 AM
dexonsmith requested changes to D45772: [Minor patch] Fix IR Module Printing.

That doesn’t seem like the right place for the newline; instead, it should be wherever the asterisks were printed.

Apr 23 2018, 6:27 AM

Apr 22 2018

dexonsmith requested changes to D30882: Add a callback for __has_include and use it for dependency scanning.

I don't think this is quite right. I know at least make-based incremental builds wouldn't deal well with this.

Apr 22 2018, 6:54 PM