In D45024#1055680, @JDevlieghere wrote:

I guess this fixes https://llvm.org/PR36420?

Nitpick.

I am not sure which fields of DILabel are required or optional, please double check yourself. I also feel bitcode patch can be combined with llvm assembly patch.

LGTM. However, combining this one with D45025 would be better.

Nit.

I don't understand the part of BasicBlock, the others LGTM.

Nits.

I think the test case should be a general one, not relate to any particular target.

@sdardis I have no commit access. Please commit it for me, thanks.

@kparzysz ping?

@kparzysz Could you commit this for me? Thanks!

Comment inline.

I have one minor concern, please see the inline comment.

I think this field is useful for most targets. Just a little concern about the coding style.

@theraven Could you commit this for me? I will be happy to update the context if I have more experience on this.

@theraven, ping?

@theraven For `libunwind` part, I am not sure if it's okay to include your link there. I just skim through the patch and list the key points I thought.

@dblaikie Done. I need your help here since I don't have commit access. Thanks. :)

@dblaikie ping? :-)

@dblaikie Done.

@dblaikie I upload the profile data and test case. However, the error message I check including the full path of the profile data. Any idea on how can I improve this?

In D35586#814443, @dblaikie wrote:

Test case?

address review comment.

In D34856#796437, @aqjune wrote:

I added 4 tests, and checked that make check-llvm works well.

@kparzysz please help me commit it as I have no commit access, thanks.

In D33281#760178, @dberlin wrote:

It looks, at least to me, like graphviz.org has mac downloads and is official.
Is there a reason not to use that?

In D32929#760181, @dberlin wrote:

This is obvious, please feel free to commit things like this in the future without review.

@grosser I add you as reviewer because you once responded my mail [1].

@dberris Thanks for the info!

In D32964#750942, @spatel wrote:

How about we add another hardware example and group those together:

Add more example.

In D31578#716833, @t.p.northover wrote:

Looks reasonable to me.

@spatel @aemerson Done. If everything looks fine, please help me commit it as I have no commit access, thanks.

In D32759#748916, @efriedma wrote:

In D32759#748653, @chenwj wrote:

In D32759#748007, @efriedma wrote:

The difference between returning true and false here is just the way error recovery works: when we return true, we know the type is invalid, so we suppress it, and subsequent errors involving the declaration. Example (Objective-C++) where we currently print two errors:

So when we see T->isObjCObjectType() is true, then we should return true since the return type is invalid?

Yes.

In D32759#748007, @efriedma wrote:

The difference between returning true and false here is just the way error recovery works: when we return true, we know the type is invalid, so we suppress it, and subsequent errors involving the declaration. Example (Objective-C++) where we currently print two errors:

@erichkeane Just share what I investigated.

@eli.friedman I find you added isObjCObjectType check in svn revision 184006 (git commit ddb5a392). Could you confirm returning zero rather than true here is okay? A little explanation would be even better. Thanks.

The GettingStarted.rst already been updated.

@t.p.northover Ping? :-)

I have no right to commit, need someone's help. :-)

@kparzysz There are two places I think using auto should be okay, replacing other auto with type.

@kparzysz All done. :-)

@spatel I update the code. please take a look, thanks. :-)

Use SDep & or const SDep & rather than auto.

Ah, sorry. "Our Tests" means the lit test SemaObjC/method-bad-param.m (line 11). I ran the lit tests initially with a breakpoint on this line and it never hit, though I must have set up the debugger wrong. Once I replaced it with an assert, method-bad-param failed.

In D32117#743341, @asb wrote:

Patch committed, many thanks for your contribution @chenwj.

In D32682#742714, @spatel wrote:

IMO, this is going beyond LLVM's normal 'auto' usage prefs:
http://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable

If you can convert it to a range-loop, then you should specify the range element type including 'const' where appropriate. So something like this:

for (const SDValue &Op : NI->op_values())

Some of the diffs use regular loops and only replace the iterator type with an 'auto'. That should be a separate NFC patch. But it's not clear to me if that also goes too far into 'auto'.

@asb Ping? :-)

@wenbos Could you explain why moving GetNumNodeResults up ahead solves the problem? I see one problem is Operator will be changed if it's SubClassOf("Intrinsic"), that might make GetNumNodeResults return the wrong result. I don't know if your patch is the right fix, although it works.

In D32117#740495, @asb wrote:

Thank you @chenwj, this is definitely an improvement over the current documentation. Would you like me to commit?

@asb And you? :-)

I don't have commit access, need your help. ;-)

In D32117#736622, @asb wrote:

Hi, thanks for the patch. My feedback is:

1. Limitations of literal formats shouldn't be discussed when introducing the types.
2. Perhaps outside the scope of this patch, but it does seem a little odd that a simple string literal can be assigned to either a string or code variable, but code fragment literals can't be assigned to string variables (at least on the couple of months old build on LLVM I have on this machine). The existing description that a code fragment is "just a multiline string literal" seems incorrect due to this.

In D32117#736630, @kparzysz wrote:

Also, does the document mention expressions like "Inst{31,27-25,14}", where the bit accesses are in a comma-separated list?

Address review comments.

Ping? :-)

Ping? Any comment? :-)

fix typo.

Ping? :-)

I don't have commit access, need someone's help. :-)

I don't have commit access, need someone's help.

I don't have commit access, need someone's help.

In D6011#4, @t.p.northover wrote:

I think this looks fine, having chased down the default arguments. But just to make sure: we both think the verifier will default to dbgs() for its output after this change (mainly rather than /dev/null)?

If so, go for it (and thanks for working on the update).

Tim.