[Lldb-commits] [PATCH] D146412: [NFC] Fix potential for use-after-free in DumpModuleInfoAction

[Tom Honermann via Phabricator via lldb-commits]

tahonermann added inline comments.



Comment at: lldb/source/Commands/CommandObjectTarget.cpp:2182-2183
 
-clang::DumpModuleInfoAction dump_module_info;
-dump_module_info.OutputStream = &result.GetOutputStream().AsRawOstream();
+std::shared_ptr Out(
+&result.GetOutputStream().AsRawOstream(), [](llvm::raw_ostream *) {});
+clang::DumpModuleInfoAction dump_module_info(Out);

Use of `std::shared_ptr` with a deleter that doesn't do anything is unusual; I 
think this deserves a comment.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146412/new/

https://reviews.llvm.org/D146412

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

[Lldb-commits] [PATCH] D146412: [NFC] Fix potential for use-after-free in DumpModuleInfoAction

[Tom Honermann via Phabricator via lldb-commits]

tahonermann accepted this revision.
tahonermann added a comment.

Thank you, Mariya! Looks good to me!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146412/new/

https://reviews.llvm.org/D146412

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

[Lldb-commits] [PATCH] D150291: [Clang] Rename internal type identifier(s) for `__bf16` to `BF16Ty`

[Tom Honermann via Phabricator via lldb-commits]

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

This looks great, thank you for doing this!

Requested changes are just to undo some of the style changes.




Comment at: clang/include/clang/Basic/Specifiers.h:82-89
+TST_class, // C++ class type
+TST_interface, // C++ (Microsoft-specific) __interface type
+TST_typename,  // Typedef, C++ class-name or enum name, etc.
 TST_typeofType,// C2x (and GNU extension) typeof(type-name)
 TST_typeofExpr,// C2x (and GNU extension) typeof(expression)
 TST_typeof_unqualType, // C2x typeof_unqual(type-name)
 TST_typeof_unqualExpr, // C2x typeof_unqual(expression)

Whitespace changes to code that is otherwise not being changed are discouraged 
due to the impact on tools like `git blame`. If you want to make such a change, 
please do so as a separate commit and add the commit to 
`.git-blame-ignore-revs`.



Comment at: clang/lib/AST/ASTContext.cpp:7075-7076
   case BuiltinType::Float128:   return Float128Rank;
-  case BuiltinType::BFloat16:   return BFloat16Rank;
+  case BuiltinType::BF16:
+return BF16Rank;
   case BuiltinType::Ibm128: return Ibm128Rank;

Please keep the same style here (I know `clang-format` might want to do 
differently; just ignore it in this case).



Comment at: clang/lib/AST/ItaniumMangle.cpp:3617-3619
+case BuiltinType::BF16:
+  EltName = "bfloat16_t";
+  break;

Likewise, please maintain consistent style here.

Thank you for *not* changing the literal! :)



Comment at: clang/lib/Sema/DeclSpec.cpp:590-591
   case DeclSpec::TST_atomic: return "_Atomic";
-  case DeclSpec::TST_BFloat16: return "__bf16";
+  case DeclSpec::TST_BF16:
+return "__bf16";
 #define GENERIC_IMAGE_TYPE(ImgType, Id) \

Please maintain consistent style here.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D150291/new/

https://reviews.llvm.org/D150291

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

[Lldb-commits] [PATCH] D150291: [Clang] Rename internal type identifier(s) for __bf16 to BF16Ty

[Tom Honermann via Phabricator via lldb-commits]

tahonermann added subscribers: lenary, foad.
tahonermann added a comment.

> I was following the LLVM contribution guidelines to use git clang-format, but 
> I understand the importance of maintaining existing code styles that may be 
> altered by git-clang format.

The guidelines are slightly in conflict in that regard so, yeah, its a 
judgement call.

I added two more suggested edits targeting some comments.

This looks good to me, but I think we should make sure AMD and ARM folks are 
aware of the change. @foad, @lenary, any concerns?




Comment at: clang/include/clang/Basic/TargetInfo.h:650
 
-  /// Determine whether the _BFloat16 type is supported on this target.
-  virtual bool hasBFloat16Type() const { return HasBFloat16; }
+  /// Determine whether the _BF16 type is supported on this target.
+  virtual bool hasBF16Type() const { return HasBF16; }





Comment at: clang/include/clang/Basic/arm_neon_incl.td:240
 // F: change to floating category.
-// B: change to BFloat16
+// B: change to BF16
 // P: change to polynomial category.




CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D150291/new/

https://reviews.llvm.org/D150291

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

[Lldb-commits] [PATCH] D150291: [Clang] Rename internal type identifier(s) for __bf16 to BF16Ty

[Tom Honermann via Phabricator via lldb-commits]

tahonermann added a subscriber: clang-vendors.
tahonermann added a comment.

Heads up @clang-vendors. This patch changes the names of many internal 
identifiers for enumerators, class data members, and functions (including 
virtual functions) that are related to support for the `__bf16` type. The 
changes are roughly to replace "bfloat16" with "bf16" to make the association 
obvious and to avoid confusion when support for the C++ `std::bfloat16_t` type 
lands (and to make room for a potential matching `_BFloat16` type in C in the 
future). There are no changes to ABI or code generation; mangling continues to 
use the same names even where those have "bfloat16" in them. These changes will 
likely cause compilation failures in downstream projects that will require 
(simple) identifier updates to resolve.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D150291/new/

https://reviews.llvm.org/D150291

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

[Lldb-commits] [PATCH] D150291: [Clang] Rename internal type identifier(s) for __bf16 to BF16Ty

[Tom Honermann via Phabricator via lldb-commits]

tahonermann accepted this revision.
tahonermann added a comment.
This revision is now accepted and ready to land.

Thanks for all the updates @codemzs! I'm going to go ahead and accept. But 
please wait a few days for recently subscribed folks to have a chance to 
comment before landing this.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D150291/new/

https://reviews.llvm.org/D150291

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

[Lldb-commits] [PATCH] D150291: [Clang] Rename internal type identifier(s) for __bf16 to BF16Ty

[Tom Honermann via Phabricator via lldb-commits]

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

> I do wonder if we need two bfloat implementations, but for that I'll leave a 
> comment on D149573 .

Given the discussions occurring in D149573 , 
let's hold off on landing this for now. It is sounding like we might have 
direction for repurposing `__bf16` for `std::bfloat16_t` (and a future 
`_BFloat16` C type; with `__bf16` retained as an alternate spelling). If we go 
in that direction, then we would presumably want a change that goes in the 
opposite direction of this patch; a change that migrates "bf16" names towards 
"bfloat16". Let's focus on confirming that direction first. I'll mark this as 
requesting changes for now while we figure this out.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D150291/new/

https://reviews.llvm.org/D150291

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