Re: [Lldb-commits] [PATCH] D15326: Rework breakpoint language filtering to use the symbol context's language.

[Jim Ingham via lldb-commits]

jingham added a subscriber: jingham.
jingham added a comment.

This patch is not acceptable as is.  It enshrines an incorrect usage of the 
function cstring_is_mangled which we shouldn’t do.  As I said in the previous 
comment, it’s okay to come up with a short term fix, and check the mangled 
flavor against Itanium and MSVC.  So it shouldn’t be hard to replace the errant 
usage.  Do that and its fine to go in.

Jim


Repository:
  rL LLVM

http://reviews.llvm.org/D15326



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

Re: [Lldb-commits] [PATCH] D15326: Rework breakpoint language filtering to use the symbol context's language.

[Jim Ingham via lldb-commits]

This patch is not acceptable as is.  It enshrines an incorrect usage of the 
function cstring_is_mangled which we shouldn’t do.  As I said in the previous 
comment, it’s okay to come up with a short term fix, and check the mangled 
flavor against Itanium and MSVC.  So it shouldn’t be hard to replace the errant 
usage.  Do that and its fine to go in.

Jim

> On Dec 10, 2015, at 4:14 PM, Dawn Perchik  wrote:
> 
> dawn added a comment.
> 
> ping?
> 
> 
> Repository:
>  rL LLVM
> 
> http://reviews.llvm.org/D15326
> 
> 
> 

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

Re: [Lldb-commits] [PATCH] D15326: Rework breakpoint language filtering to use the symbol context's language.

[Dawn Perchik via lldb-commits]

dawn added a comment.

ping?


Repository:
  rL LLVM

http://reviews.llvm.org/D15326



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

Re: [Lldb-commits] [PATCH] D15326: Rework breakpoint language filtering to use the symbol context's language.

[Jim Ingham via lldb-commits]

jingham added a comment.

"cstring_is_mangled" does NOT mean that the string is a C++ mangled string, it 
means it is ANY kind of mangled string - you can tell because it is used to 
determine whether to populate the m_mangled and m_demangled components of the 
Mangled structure, which it should do regardless of the language the compiler 
happens to be mangling .  If you look at the Github sources with Swift support, 
you will see that this returns true for both C++ mangled names and Swift 
mangled names, as it should.

It looks like you are using a function that had this error originally, so it is 
not your fault per se.  We were probably getting away with it because it was 
never used in a case where its result mattered (it isn't used much...)  But you 
are now using it somewhere where the incorrect usage would matter.  So it needs 
to be fixed before the patch is correct.

For the nonce, it is probably good enough for Mangled::GuessLanguage to return 
C++ if the mangling scheme is Itanium or MSVC.  We definitely have to figure 
out a better way to this, so a comment to that effect would also be in order.


Repository:
  rL LLVM

http://reviews.llvm.org/D15326



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

Re: [Lldb-commits] [PATCH] D15326: Rework breakpoint language filtering to use the symbol context's language.

[Jim Ingham via lldb-commits]

jingham requested changes to this revision.
jingham added a comment.
This revision now requires changes to proceed.

The part of this fix which is using info in the SymbolContext to make the 
language detection more accurate is fine.

We have to do something better about how we detect the language from a mangled 
name.  The target knows some that it should be able to contribute, but there's 
no clear way to do that now.  But you aren't making that any worse (except the 
one inline comment about cstring_is_mangled above, which will need to get 
fixed) so I don't think you are obligated to solve that problem now.


Repository:
  rL LLVM

http://reviews.llvm.org/D15326



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

Re: [Lldb-commits] [PATCH] D15326: Rework breakpoint language filtering to use the symbol context's language.

[Dawn Perchik via lldb-commits]

dawn added a reviewer: clayborg.
dawn added a comment.

(Added Greg - he wrote Mangled::GetLanguage - now GuessLanguage).

This patch removes the dependence on determining language from the name 
mangling for 99% of cases (there's still the much less common problem for 
symbols since Symbol 's GetLanguage calls Mangled::GuessLanguage).  It is a win 
win.


Repository:
  rL LLVM

http://reviews.llvm.org/D15326



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

Re: [Lldb-commits] [PATCH] D15326: Rework breakpoint language filtering to use the symbol context's language.

[Dawn Perchik via lldb-commits]

dawn marked an inline comment as done.
dawn added a comment.

Thanks Greg.  Will fix comment in commit.

@Jim, can you accept please?


Repository:
  rL LLVM

http://reviews.llvm.org/D15326



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

Re: [Lldb-commits] [PATCH] D15326: Rework breakpoint language filtering to use the symbol context's language.

[Dawn Perchik via lldb-commits]

dawn requested a review of this revision.
dawn added a comment.

Please reconsider.  Thanks.


Repository:
  rL LLVM

http://reviews.llvm.org/D15326



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