[PATCH] D142804: [clang-format] Support clang-format on/off line comments as prefixes

[Alex via Phabricator via cfe-commits]

alexolog added inline comments.



Comment at: clang/lib/Format/Format.cpp:3893
 
+bool isClangFormatOn(StringRef Comment) {
+  if (Comment == "/* clang-format on */")

Here's my attempt at something flexible:
Disclaimer: this is my first time looking at the LLVM codebase, so don't expect 
production quality.

```


// Implementation detail, hide in a namespace and/or take out of the header
bool isClangFormatMarked(StringRef Comment, StringRef Mark) {
  // Quick heuristics: minimum length and starts with a slash (comment)
  // Shortest tag: "//clang-format on", 17 characters
  static constexpr StringLiteral clangFormatStr("clang-format ");
  if (Comment.size() < clangFormatStr.size() + 4 || Comment[0] != '/')
return false;

  // check if it's a comment starting with "//" or "/*"
  bool CloseNeeded = false;
  if (Comment[1] == '*')
CloseNeeded = true;
  else if (Comment[1] != '/')
return false;

  // Remove the comment start and all following whitespace
  Comment = Comment.substr(2).ltrim();

  // Check for the actual command, a piece at a time
  if (!Comment.consume_front(clangFormatStr) || !Comment.consume_front(Mark))
return false;

  // Are we there yet?
  if (!CloseNeeded && Comment.empty() ||
  CloseNeeded && Comment.starts_with("*/"))
return true;

  // For a trailer, restrict the next character
  // (currently spaces and tabs, but can include a colon, etc.)
  static constexpr StringLiteral Separator(" \t");
  if (!Separator.contains(Comment[0]))
return false;
  
  // Verify comment is properly terminated
  if (!CloseNeeded || Comment.contains("*/"))
return true;

  return false; // Everything else
}



bool isClangFormatOn(StringRef Comment) {
  return isClangFormatMarked(Comment, "on");
}

bool isClangFormatOff(StringRef Comment) {
  return isClangFormatMarked(Comment, "off");
}



  EXPECT_TRUE(isClangFormatOn("//clang-format on"));
  EXPECT_TRUE(isClangFormatOn("// clang-format on"));
  EXPECT_TRUE(isClangFormatOn("//clang-format on "));
  EXPECT_TRUE(isClangFormatOn("//clang-format on and off"));
  EXPECT_TRUE(isClangFormatOn("/*clang-format on*/"));
  EXPECT_TRUE(isClangFormatOn("/* clang-format on*/"));
  EXPECT_TRUE(isClangFormatOn("/*clang-format on */"));
  EXPECT_TRUE(isClangFormatOn("/*clang-format on*/int i{0};"));

  EXPECT_FALSE(isClangFormatOn("//clang-format  on"));
  EXPECT_FALSE(isClangFormatOn("//clang-format o"));
  EXPECT_FALSE(isClangFormatOn("// clang-format o"));
  EXPECT_FALSE(isClangFormatOn("//clang-format ontario"));
  EXPECT_FALSE(isClangFormatOn("//clang-format off"));
  EXPECT_FALSE(isClangFormatOn("/*clang-format onwards*/"));


```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142804

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

[PATCH] D142804: [clang-format] Support clang-format on/off line comments as prefixes

[Alex via Phabricator via cfe-commits]

alexolog added inline comments.



Comment at: clang/lib/Format/Format.cpp:3893
 
+bool isClangFormatOn(StringRef Comment) {
+  if (Comment == "/* clang-format on */")

alexolog wrote:
> Here's my attempt at something flexible:
> Disclaimer: this is my first time looking at the LLVM codebase, so don't 
> expect production quality.
> 
> ```
> 
> 
> // Implementation detail, hide in a namespace and/or take out of the header
> bool isClangFormatMarked(StringRef Comment, StringRef Mark) {
>   // Quick heuristics: minimum length and starts with a slash (comment)
>   // Shortest tag: "//clang-format on", 17 characters
>   static constexpr StringLiteral clangFormatStr("clang-format ");
>   if (Comment.size() < clangFormatStr.size() + 4 || Comment[0] != '/')
> return false;
> 
>   // check if it's a comment starting with "//" or "/*"
>   bool CloseNeeded = false;
>   if (Comment[1] == '*')
> CloseNeeded = true;
>   else if (Comment[1] != '/')
> return false;
> 
>   // Remove the comment start and all following whitespace
>   Comment = Comment.substr(2).ltrim();
> 
>   // Check for the actual command, a piece at a time
>   if (!Comment.consume_front(clangFormatStr) || !Comment.consume_front(Mark))
> return false;
> 
>   // Are we there yet?
>   if (!CloseNeeded && Comment.empty() ||
>   CloseNeeded && Comment.starts_with("*/"))
> return true;
> 
>   // For a trailer, restrict the next character
>   // (currently spaces and tabs, but can include a colon, etc.)
>   static constexpr StringLiteral Separator(" \t");
>   if (!Separator.contains(Comment[0]))
> return false;
>   
>   // Verify comment is properly terminated
>   if (!CloseNeeded || Comment.contains("*/"))
> return true;
> 
>   return false; // Everything else
> }
> 
> 
> 
> bool isClangFormatOn(StringRef Comment) {
>   return isClangFormatMarked(Comment, "on");
> }
> 
> bool isClangFormatOff(StringRef Comment) {
>   return isClangFormatMarked(Comment, "off");
> }
> 
> 
> 
>   EXPECT_TRUE(isClangFormatOn("//clang-format on"));
>   EXPECT_TRUE(isClangFormatOn("// clang-format on"));
>   EXPECT_TRUE(isClangFormatOn("//clang-format on "));
>   EXPECT_TRUE(isClangFormatOn("//clang-format on and off"));
>   EXPECT_TRUE(isClangFormatOn("/*clang-format on*/"));
>   EXPECT_TRUE(isClangFormatOn("/* clang-format on*/"));
>   EXPECT_TRUE(isClangFormatOn("/*clang-format on */"));
>   EXPECT_TRUE(isClangFormatOn("/*clang-format on*/int i{0};"));
> 
>   EXPECT_FALSE(isClangFormatOn("//clang-format  on"));
>   EXPECT_FALSE(isClangFormatOn("//clang-format o"));
>   EXPECT_FALSE(isClangFormatOn("// clang-format o"));
>   EXPECT_FALSE(isClangFormatOn("//clang-format ontario"));
>   EXPECT_FALSE(isClangFormatOn("//clang-format off"));
>   EXPECT_FALSE(isClangFormatOn("/*clang-format onwards*/"));
> 
> 
> ```
Sorry about the "done".  My misunderstanding


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142804

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

[PATCH] D142804: [clang-format] Support clang-format on/off line comments as prefixes

[Alex via Phabricator via cfe-commits]

alexolog added inline comments.



Comment at: clang/lib/Format/Format.cpp:3893
 
+bool isClangFormatOn(StringRef Comment) {
+  if (Comment == "/* clang-format on */")

owenpan wrote:
> alexolog wrote:
> > alexolog wrote:
> > > Here's my attempt at something flexible:
> > > Disclaimer: this is my first time looking at the LLVM codebase, so don't 
> > > expect production quality.
> > > 
> > > ```
> > > 
> > > 
> > > // Implementation detail, hide in a namespace and/or take out of the 
> > > header
> > > bool isClangFormatMarked(StringRef Comment, StringRef Mark) {
> > >   // Quick heuristics: minimum length and starts with a slash (comment)
> > >   // Shortest tag: "//clang-format on", 17 characters
> > >   static constexpr StringLiteral clangFormatStr("clang-format ");
> > >   if (Comment.size() < clangFormatStr.size() + 4 || Comment[0] != '/')
> > > return false;
> > > 
> > >   // check if it's a comment starting with "//" or "/*"
> > >   bool CloseNeeded = false;
> > >   if (Comment[1] == '*')
> > > CloseNeeded = true;
> > >   else if (Comment[1] != '/')
> > > return false;
> > > 
> > >   // Remove the comment start and all following whitespace
> > >   Comment = Comment.substr(2).ltrim();
> > > 
> > >   // Check for the actual command, a piece at a time
> > >   if (!Comment.consume_front(clangFormatStr) || 
> > > !Comment.consume_front(Mark))
> > > return false;
> > > 
> > >   // Are we there yet?
> > >   if (!CloseNeeded && Comment.empty() ||
> > >   CloseNeeded && Comment.starts_with("*/"))
> > > return true;
> > > 
> > >   // For a trailer, restrict the next character
> > >   // (currently spaces and tabs, but can include a colon, etc.)
> > >   static constexpr StringLiteral Separator(" \t");
> > >   if (!Separator.contains(Comment[0]))
> > > return false;
> > >   
> > >   // Verify comment is properly terminated
> > >   if (!CloseNeeded || Comment.contains("*/"))
> > > return true;
> > > 
> > >   return false; // Everything else
> > > }
> > > 
> > > 
> > > 
> > > bool isClangFormatOn(StringRef Comment) {
> > >   return isClangFormatMarked(Comment, "on");
> > > }
> > > 
> > > bool isClangFormatOff(StringRef Comment) {
> > >   return isClangFormatMarked(Comment, "off");
> > > }
> > > 
> > > 
> > > 
> > >   EXPECT_TRUE(isClangFormatOn("//clang-format on"));
> > >   EXPECT_TRUE(isClangFormatOn("// clang-format on"));
> > >   EXPECT_TRUE(isClangFormatOn("//clang-format on "));
> > >   EXPECT_TRUE(isClangFormatOn("//clang-format on and off"));
> > >   EXPECT_TRUE(isClangFormatOn("/*clang-format on*/"));
> > >   EXPECT_TRUE(isClangFormatOn("/* clang-format on*/"));
> > >   EXPECT_TRUE(isClangFormatOn("/*clang-format on */"));
> > >   EXPECT_TRUE(isClangFormatOn("/*clang-format on*/int i{0};"));
> > > 
> > >   EXPECT_FALSE(isClangFormatOn("//clang-format  on"));
> > >   EXPECT_FALSE(isClangFormatOn("//clang-format o"));
> > >   EXPECT_FALSE(isClangFormatOn("// clang-format o"));
> > >   EXPECT_FALSE(isClangFormatOn("//clang-format ontario"));
> > >   EXPECT_FALSE(isClangFormatOn("//clang-format off"));
> > >   EXPECT_FALSE(isClangFormatOn("/*clang-format onwards*/"));
> > > 
> > > 
> > > ```
> > Sorry about the "done".  My misunderstanding
> > Here's my attempt at something flexible:
> > Disclaimer: this is my first time looking at the LLVM codebase, so don't 
> > expect production quality.
> 
> Thanks! If we didn't have to worry about regressions, we might want to do 
> something like what you suggested above.
Isn't it what extensive test coverage is for?


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

https://reviews.llvm.org/D142804

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

[PATCH] D118011: [RISCV] Adjust predicates and update intrinsic for clmul and clmulh in Zbkc extension

[Alex via Phabricator via cfe-commits]

alextsao1999 added a comment.

In D118011#3265374 , @Jimerlife wrote:

> In D118011#3265071 , @craig.topper 
> wrote:
>
>> Will you also be updating clang's RISCVBuiltins.def?
>>
>> Can you add tests?
>
> I update BuiltinsRISCV.def and add "__builtin_riscv_clmul_kc" intrinsic for 
> zbkc extension, but I am not sure if the name is  appropriate.

https://github.com/rvkrypto/rvkrypto-fips/blob/main/rvkintrin.md
Is the name as `__builtin_riscv_clmul_32` or `__builtin_riscv_clmul_64` better 
for conflict resolution?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118011

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

[PATCH] D116722: [clang] Verify ssp buffer size is a valid integer

[Alex via Phabricator via cfe-commits]

alextsao1999 created this revision.
alextsao1999 added a reviewer: compnerd.
alextsao1999 requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

This patch adds to verify whether the ssp buffer size is a legal integer and a 
new diagnostic driver kind has been added.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D116722

Files:
  clang/include/clang/Basic/DiagnosticDriverKinds.td
  clang/lib/Driver/ToolChains/Clang.cpp


Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -3187,14 +3187,26 @@
 CmdArgs.push_back(Args.MakeArgString(Twine(StackProtectorLevel)));
   }
 
+  auto IsInteger = [](StringRef Str) -> bool {
+if (Str.empty())
+  return false;
+for (auto &Chr : Str)
+  if (Chr < '0' || Chr > '9')
+return false;
+return true;
+  };
+
   // --param ssp-buffer-size=
   for (const Arg *A : Args.filtered(options::OPT__param)) {
 StringRef Str(A->getValue());
 if (Str.startswith("ssp-buffer-size=")) {
   if (StackProtectorLevel) {
-CmdArgs.push_back("-stack-protector-buffer-size");
-// FIXME: Verify the argument is a valid integer.
-CmdArgs.push_back(Args.MakeArgString(Str.drop_front(16)));
+auto BufferSize = Str.drop_front(16);
+if (IsInteger(BufferSize)) {
+  CmdArgs.push_back("-stack-protector-buffer-size");
+  CmdArgs.push_back(Args.MakeArgString(BufferSize));
+} else
+  D.Diag(clang::diag::err_invalid_ssp_buffer_size);
   }
   A->claim();
 }
Index: clang/include/clang/Basic/DiagnosticDriverKinds.td
===
--- clang/include/clang/Basic/DiagnosticDriverKinds.td
+++ clang/include/clang/Basic/DiagnosticDriverKinds.td
@@ -278,6 +278,8 @@
   DefaultError;
 def err_invalid_macos_32bit_deployment_target : Error<
   "32-bit targets are not supported when building for Mac Catalyst">;
+def err_invalid_ssp_buffer_size : Error<
+  "ssp buffer size is not valid">;
 def err_drv_invalid_os_in_arg : Error<"invalid OS value '%0' in '%1'">;
 def err_drv_conflicting_deployment_targets : Error<
   "conflicting deployment targets, both '%0' and '%1' are present in 
environment">;


Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -3187,14 +3187,26 @@
 CmdArgs.push_back(Args.MakeArgString(Twine(StackProtectorLevel)));
   }
 
+  auto IsInteger = [](StringRef Str) -> bool {
+if (Str.empty())
+  return false;
+for (auto &Chr : Str)
+  if (Chr < '0' || Chr > '9')
+return false;
+return true;
+  };
+
   // --param ssp-buffer-size=
   for (const Arg *A : Args.filtered(options::OPT__param)) {
 StringRef Str(A->getValue());
 if (Str.startswith("ssp-buffer-size=")) {
   if (StackProtectorLevel) {
-CmdArgs.push_back("-stack-protector-buffer-size");
-// FIXME: Verify the argument is a valid integer.
-CmdArgs.push_back(Args.MakeArgString(Str.drop_front(16)));
+auto BufferSize = Str.drop_front(16);
+if (IsInteger(BufferSize)) {
+  CmdArgs.push_back("-stack-protector-buffer-size");
+  CmdArgs.push_back(Args.MakeArgString(BufferSize));
+} else
+  D.Diag(clang::diag::err_invalid_ssp_buffer_size);
   }
   A->claim();
 }
Index: clang/include/clang/Basic/DiagnosticDriverKinds.td
===
--- clang/include/clang/Basic/DiagnosticDriverKinds.td
+++ clang/include/clang/Basic/DiagnosticDriverKinds.td
@@ -278,6 +278,8 @@
   DefaultError;
 def err_invalid_macos_32bit_deployment_target : Error<
   "32-bit targets are not supported when building for Mac Catalyst">;
+def err_invalid_ssp_buffer_size : Error<
+  "ssp buffer size is not valid">;
 def err_drv_invalid_os_in_arg : Error<"invalid OS value '%0' in '%1'">;
 def err_drv_conflicting_deployment_targets : Error<
   "conflicting deployment targets, both '%0' and '%1' are present in environment">;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D116722: [clang] Verify ssp buffer size is a valid integer

[Alex via Phabricator via cfe-commits]

alextsao1999 updated this revision to Diff 398354.
alextsao1999 added a comment.

update


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116722

Files:
  clang/include/clang/Basic/DiagnosticDriverKinds.td
  clang/lib/Driver/ToolChains/Clang.cpp


Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -3187,14 +3187,27 @@
 CmdArgs.push_back(Args.MakeArgString(Twine(StackProtectorLevel)));
   }
 
+  auto IsInteger = [](StringRef Str) -> bool {
+if (Str.empty())
+  return false;
+for (auto &Chr : Str)
+  if (Chr < '0' || Chr > '9')
+return false;
+return true;
+  };
+
   // --param ssp-buffer-size=
   for (const Arg *A : Args.filtered(options::OPT__param)) {
 StringRef Str(A->getValue());
-if (Str.startswith("ssp-buffer-size=")) {
+auto StrSplit = Str.split('=');
+if (StrSplit.first.equals("ssp-buffer-size")) {
   if (StackProtectorLevel) {
-CmdArgs.push_back("-stack-protector-buffer-size");
-// FIXME: Verify the argument is a valid integer.
-CmdArgs.push_back(Args.MakeArgString(Str.drop_front(16)));
+if (IsInteger(StrSplit.second)) {
+  CmdArgs.push_back("-stack-protector-buffer-size");
+  CmdArgs.push_back(Args.MakeArgString(StrSplit.second));
+} else {
+  D.Diag(clang::diag::err_invalid_ssp_buffer_size);
+}
   }
   A->claim();
 }
Index: clang/include/clang/Basic/DiagnosticDriverKinds.td
===
--- clang/include/clang/Basic/DiagnosticDriverKinds.td
+++ clang/include/clang/Basic/DiagnosticDriverKinds.td
@@ -278,6 +278,8 @@
   DefaultError;
 def err_invalid_macos_32bit_deployment_target : Error<
   "32-bit targets are not supported when building for Mac Catalyst">;
+def err_invalid_ssp_buffer_size : Error<
+  "ssp buffer size is not valid">;
 def err_drv_invalid_os_in_arg : Error<"invalid OS value '%0' in '%1'">;
 def err_drv_conflicting_deployment_targets : Error<
   "conflicting deployment targets, both '%0' and '%1' are present in 
environment">;


Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -3187,14 +3187,27 @@
 CmdArgs.push_back(Args.MakeArgString(Twine(StackProtectorLevel)));
   }
 
+  auto IsInteger = [](StringRef Str) -> bool {
+if (Str.empty())
+  return false;
+for (auto &Chr : Str)
+  if (Chr < '0' || Chr > '9')
+return false;
+return true;
+  };
+
   // --param ssp-buffer-size=
   for (const Arg *A : Args.filtered(options::OPT__param)) {
 StringRef Str(A->getValue());
-if (Str.startswith("ssp-buffer-size=")) {
+auto StrSplit = Str.split('=');
+if (StrSplit.first.equals("ssp-buffer-size")) {
   if (StackProtectorLevel) {
-CmdArgs.push_back("-stack-protector-buffer-size");
-// FIXME: Verify the argument is a valid integer.
-CmdArgs.push_back(Args.MakeArgString(Str.drop_front(16)));
+if (IsInteger(StrSplit.second)) {
+  CmdArgs.push_back("-stack-protector-buffer-size");
+  CmdArgs.push_back(Args.MakeArgString(StrSplit.second));
+} else {
+  D.Diag(clang::diag::err_invalid_ssp_buffer_size);
+}
   }
   A->claim();
 }
Index: clang/include/clang/Basic/DiagnosticDriverKinds.td
===
--- clang/include/clang/Basic/DiagnosticDriverKinds.td
+++ clang/include/clang/Basic/DiagnosticDriverKinds.td
@@ -278,6 +278,8 @@
   DefaultError;
 def err_invalid_macos_32bit_deployment_target : Error<
   "32-bit targets are not supported when building for Mac Catalyst">;
+def err_invalid_ssp_buffer_size : Error<
+  "ssp buffer size is not valid">;
 def err_drv_invalid_os_in_arg : Error<"invalid OS value '%0' in '%1'">;
 def err_drv_conflicting_deployment_targets : Error<
   "conflicting deployment targets, both '%0' and '%1' are present in environment">;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits