[PATCH] D45045: [DebugInfo] Generate debug information about labels

2018-03-30 Thread Wei-Ren Chen via Phabricator via cfe-commits 
chenwj requested changes to this revision.
chenwj added a comment.
This revision now requires changes to proceed.

Nits.




Comment at: lib/CodeGen/CGDebugInfo.cpp:3644
+void CGDebugInfo::EmitLabel(const LabelDecl *D,
+   CGBuilderTy &Builder) {
+  assert(DebugKind >= codegenoptions::LimitedDebugInfo);

Indent.



Comment at: lib/CodeGen/CGStmt.cpp:535
+
+  // Emit debug info for label.
+  if (HaveInsertPoint())

I assume you emit debug info for the label only if it's reachable by checking 
`HaveInsertPoint()`. If so, make the comment as


```
// Emit debug info for the label only if it's reachable.
```

I prefer adding braces here. Please check indent as well.


Repository:
  rC Clang

https://reviews.llvm.org/D45045



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D32014: Remove unused varible

2017-04-13 Thread Wei-Ren Chen via Phabricator via cfe-commits 
chenwj created this revision.
Herald added subscribers: rengolin, aemerson.

The Result variable is unused both in Sema::CheckARMBuiltinFunctionCall and 
Sema::CheckAArch64BuiltinFunctionCall, remove it.


https://reviews.llvm.org/D32014

Files:
  lib/Sema/SemaChecking.cpp


Index: lib/Sema/SemaChecking.cpp
===
--- lib/Sema/SemaChecking.cpp
+++ lib/Sema/SemaChecking.cpp
@@ -1391,8 +1391,6 @@
 }
 
 bool Sema::CheckARMBuiltinFunctionCall(unsigned BuiltinID, CallExpr *TheCall) {
-  llvm::APSInt Result;
-
   if (BuiltinID == ARM::BI__builtin_arm_ldrex ||
   BuiltinID == ARM::BI__builtin_arm_ldaex ||
   BuiltinID == ARM::BI__builtin_arm_strex ||
@@ -1439,8 +1437,6 @@
 
 bool Sema::CheckAArch64BuiltinFunctionCall(unsigned BuiltinID,
  CallExpr *TheCall) {
-  llvm::APSInt Result;
-
   if (BuiltinID == AArch64::BI__builtin_arm_ldrex ||
   BuiltinID == AArch64::BI__builtin_arm_ldaex ||
   BuiltinID == AArch64::BI__builtin_arm_strex ||


Index: lib/Sema/SemaChecking.cpp
===
--- lib/Sema/SemaChecking.cpp
+++ lib/Sema/SemaChecking.cpp
@@ -1391,8 +1391,6 @@
 }
 
 bool Sema::CheckARMBuiltinFunctionCall(unsigned BuiltinID, CallExpr *TheCall) {
-  llvm::APSInt Result;
-
   if (BuiltinID == ARM::BI__builtin_arm_ldrex ||
   BuiltinID == ARM::BI__builtin_arm_ldaex ||
   BuiltinID == ARM::BI__builtin_arm_strex ||
@@ -1439,8 +1437,6 @@
 
 bool Sema::CheckAArch64BuiltinFunctionCall(unsigned BuiltinID,
  CallExpr *TheCall) {
-  llvm::APSInt Result;
-
   if (BuiltinID == AArch64::BI__builtin_arm_ldrex ||
   BuiltinID == AArch64::BI__builtin_arm_ldaex ||
   BuiltinID == AArch64::BI__builtin_arm_strex ||
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D32014: Remove unused varible

2017-04-13 Thread Wei-Ren Chen via Phabricator via cfe-commits 
chenwj added a comment.

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


https://reviews.llvm.org/D32014



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D32014: Remove unused varible

2017-04-13 Thread Wei-Ren Chen via Phabricator via cfe-commits 
chenwj added a comment.

Ping? :-)


https://reviews.llvm.org/D32014



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D32759: Fix errored return value in CheckFunctionReturnType and add a fixit hint

2017-05-03 Thread Wei-Ren Chen via Phabricator via cfe-commits 
chenwj added a comment.

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

The fix LGTM. Is it possible to tweak method-bad-param.m so that we can see the 
difference after the fix?


https://reviews.llvm.org/D32759



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D32759: Fix errored return value in CheckFunctionReturnType and add a fixit hint

2017-05-05 Thread Wei-Ren Chen via Phabricator via cfe-commits 
chenwj added a subscriber: eli.friedman.
chenwj added a comment.

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


https://reviews.llvm.org/D32759



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D32759: Fix errored return value in CheckFunctionReturnType and add a fixit hint

2017-05-05 Thread Wei-Ren Chen via Phabricator via cfe-commits 
chenwj added a comment.

@erichkeane Just share what I investigated.

In https://reviews.llvm.org/D32759#744769, @erichkeane wrote:

> In https://reviews.llvm.org/D32759#744613, @chenwj wrote:
>
> > > 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.
> >
> > The fix LGTM. Is it possible to tweak method-bad-param.m so that we can see 
> > the difference after the fix?
>
>
> I actually couldn't come up with a way where we COULD see the difference...  
> I was hoiping someone else could come up with something if it were important 
> enough for us.


I fwd to @eli.friedman who added the check. Maybe he is the right person who 
can explain what's going on here, and decide if this patch is okay or not.


https://reviews.llvm.org/D32759



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D32759: Fix errored return value in CheckFunctionReturnType and add a fixit hint

2017-05-08 Thread Wei-Ren Chen via Phabricator via cfe-commits 
chenwj added a comment.

In https://reviews.llvm.org/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?


https://reviews.llvm.org/D32759



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D32759: Fix errored return value in CheckFunctionReturnType and add a fixit hint

2017-05-10 Thread Wei-Ren Chen via Phabricator via cfe-commits 
chenwj added a comment.

In https://reviews.llvm.org/D32759#748916, @efriedma wrote:

> In https://reviews.llvm.org/D32759#748653, @chenwj wrote:
>
> > In https://reviews.llvm.org/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.


@efriedma So @erichkeane 's patch looks okay?


https://reviews.llvm.org/D32759



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits