[PATCH] D148601: [Clang] Handle Error message to output proper Prefix

[Usman Akinyemi via Phabricator via cfe-commits]

Unique_Usman added a comment.

In D148601#4279334 , @tbaeder wrote:

> I am not 100% sure about the semantics of passing multiple prefixes, i.e. if 
> the error is emitted for all prefixes individually or if it's only emitted if 
> no `expected` line for any of the prefixes is found. In the latter case we 
> should probably add all the prefixes to the error message.

I tested different scenerios e.g added more than one RUN lines with different 
value of -verify, what I concluded on is that if we have multiple  RUN lines 
with each of them having no directive, the prefixes generated is always of the 
first occurence  with no  expected directive so, the error is always generated 
for the first occurence with no expected directive.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148601

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

[PATCH] D148601: [Clang] Handle Error message to output proper Prefix

[Usman Akinyemi via Phabricator via cfe-commits]

Unique_Usman updated this revision to Diff 515050.

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

https://reviews.llvm.org/D148601

Files:
  clang/include/clang/Basic/DiagnosticFrontendKinds.td
  clang/lib/Frontend/VerifyDiagnosticConsumer.cpp


Index: clang/lib/Frontend/VerifyDiagnosticConsumer.cpp
===
--- clang/lib/Frontend/VerifyDiagnosticConsumer.cpp
+++ clang/lib/Frontend/VerifyDiagnosticConsumer.cpp
@@ -1095,7 +1095,8 @@
 // Produce an error if no expected-* directives could be found in the
 // source file(s) processed.
 if (Status == HasNoDirectives) {
-  Diags.Report(diag::err_verify_no_directives).setForceEmit();
+  const auto &Prefixes = Diags.getDiagnosticOptions().VerifyPrefixes;
+  Diags.Report(diag::err_verify_no_directives).setForceEmit() << 
*Prefixes.begin();
   ++NumErrors;
   Status = HasNoDirectivesReported;
 }
Index: clang/include/clang/Basic/DiagnosticFrontendKinds.td
===
--- clang/include/clang/Basic/DiagnosticFrontendKinds.td
+++ clang/include/clang/Basic/DiagnosticFrontendKinds.td
@@ -175,7 +175,7 @@
 "%select{expected|'expected-no-diagnostics'}0 directive cannot follow "
 "%select{'expected-no-diagnostics' directive|other expected directives}0">;
 def err_verify_no_directives : Error<
-"no expected directives found: consider use of 'expected-no-diagnostics'">;
+"no expected directives found: consider use of '%0-no-diagnostics'">;
 def err_verify_nonconst_addrspace : Error<
   "qualifier 'const' is needed for variables in address space '%0'">;
 


Index: clang/lib/Frontend/VerifyDiagnosticConsumer.cpp
===
--- clang/lib/Frontend/VerifyDiagnosticConsumer.cpp
+++ clang/lib/Frontend/VerifyDiagnosticConsumer.cpp
@@ -1095,7 +1095,8 @@
 // Produce an error if no expected-* directives could be found in the
 // source file(s) processed.
 if (Status == HasNoDirectives) {
-  Diags.Report(diag::err_verify_no_directives).setForceEmit();
+  const auto &Prefixes = Diags.getDiagnosticOptions().VerifyPrefixes;
+  Diags.Report(diag::err_verify_no_directives).setForceEmit() << *Prefixes.begin();
   ++NumErrors;
   Status = HasNoDirectivesReported;
 }
Index: clang/include/clang/Basic/DiagnosticFrontendKinds.td
===
--- clang/include/clang/Basic/DiagnosticFrontendKinds.td
+++ clang/include/clang/Basic/DiagnosticFrontendKinds.td
@@ -175,7 +175,7 @@
 "%select{expected|'expected-no-diagnostics'}0 directive cannot follow "
 "%select{'expected-no-diagnostics' directive|other expected directives}0">;
 def err_verify_no_directives : Error<
-"no expected directives found: consider use of 'expected-no-diagnostics'">;
+"no expected directives found: consider use of '%0-no-diagnostics'">;
 def err_verify_nonconst_addrspace : Error<
   "qualifier 'const' is needed for variables in address space '%0'">;
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D148601: [Clang] Handle Error message to output proper Prefix

[Usman Akinyemi via Phabricator via cfe-commits]

Unique_Usman updated this revision to Diff 516762.

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

https://reviews.llvm.org/D148601

Files:
  clang/include/clang/Basic/DiagnosticFrontendKinds.td
  clang/lib/Frontend/VerifyDiagnosticConsumer.cpp


Index: clang/lib/Frontend/VerifyDiagnosticConsumer.cpp
===
--- clang/lib/Frontend/VerifyDiagnosticConsumer.cpp
+++ clang/lib/Frontend/VerifyDiagnosticConsumer.cpp
@@ -1095,7 +1095,8 @@
 // Produce an error if no expected-* directives could be found in the
 // source file(s) processed.
 if (Status == HasNoDirectives) {
-  Diags.Report(diag::err_verify_no_directives).setForceEmit();
+  const auto &Prefixes = Diags.getDiagnosticOptions().VerifyPrefixes;
+  Diags.Report(diag::err_verify_no_directives).setForceEmit() << 
*Prefixes.begin();
   ++NumErrors;
   Status = HasNoDirectivesReported;
 }
Index: clang/include/clang/Basic/DiagnosticFrontendKinds.td
===
--- clang/include/clang/Basic/DiagnosticFrontendKinds.td
+++ clang/include/clang/Basic/DiagnosticFrontendKinds.td
@@ -175,7 +175,7 @@
 "%select{expected|'expected-no-diagnostics'}0 directive cannot follow "
 "%select{'expected-no-diagnostics' directive|other expected directives}0">;
 def err_verify_no_directives : Error<
-"no expected directives found: consider use of 'expected-no-diagnostics'">;
+"no expected directives found: consider use of '%0-no-diagnostics'">;
 def err_verify_nonconst_addrspace : Error<
   "qualifier 'const' is needed for variables in address space '%0'">;
 


Index: clang/lib/Frontend/VerifyDiagnosticConsumer.cpp
===
--- clang/lib/Frontend/VerifyDiagnosticConsumer.cpp
+++ clang/lib/Frontend/VerifyDiagnosticConsumer.cpp
@@ -1095,7 +1095,8 @@
 // Produce an error if no expected-* directives could be found in the
 // source file(s) processed.
 if (Status == HasNoDirectives) {
-  Diags.Report(diag::err_verify_no_directives).setForceEmit();
+  const auto &Prefixes = Diags.getDiagnosticOptions().VerifyPrefixes;
+  Diags.Report(diag::err_verify_no_directives).setForceEmit() << *Prefixes.begin();
   ++NumErrors;
   Status = HasNoDirectivesReported;
 }
Index: clang/include/clang/Basic/DiagnosticFrontendKinds.td
===
--- clang/include/clang/Basic/DiagnosticFrontendKinds.td
+++ clang/include/clang/Basic/DiagnosticFrontendKinds.td
@@ -175,7 +175,7 @@
 "%select{expected|'expected-no-diagnostics'}0 directive cannot follow "
 "%select{'expected-no-diagnostics' directive|other expected directives}0">;
 def err_verify_no_directives : Error<
-"no expected directives found: consider use of 'expected-no-diagnostics'">;
+"no expected directives found: consider use of '%0-no-diagnostics'">;
 def err_verify_nonconst_addrspace : Error<
   "qualifier 'const' is needed for variables in address space '%0'">;
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D148601: [Clang] Handle Error message to output proper Prefix

[Usman Akinyemi via Phabricator via cfe-commits]

Unique_Usman added a comment.

In D148601#4295719 , @tbaeder wrote:

> In D148601#4279604 , @Unique_Usman 
> wrote:
>
>> In D148601#4279334 , @tbaeder 
>> wrote:
>>
>>> I am not 100% sure about the semantics of passing multiple prefixes, i.e. 
>>> if the error is emitted for all prefixes individually or if it's only 
>>> emitted if no `expected` line for any of the prefixes is found. In the 
>>> latter case we should probably add all the prefixes to the error message.
>>
>> I tested different scenerios e.g added more than one RUN lines with 
>> different value of -verify, what I concluded on is that if we have multiple  
>> RUN lines with each of them having no directive, the prefixes generated is 
>> always of the first occurence  with no  expected directive so, the error is 
>> always generated for the first occurence with no expected directive.
>
> Yeah but I think you can do `-verify=foo,bar`(?) in which case the list f 
> prefixes would actually have more than one item.

I used -verify=foo,bar but, the prefixes still have just only one item, in the 
case bar. Does this the implementation of getting the prefixes is faulty?


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

https://reviews.llvm.org/D148601

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

[PATCH] D148601: [Clang] Handle Error message to output proper Prefix

[Usman Akinyemi via Phabricator via cfe-commits]

Unique_Usman added a comment.

Thanks @tbaeder, I really appreciate your time.

Name is Unique_Usman and my email is usmanakinyemi...@gmail.com

As you mentioned, if anyone later chime in, I will be glad to update the issue 
back. 
Thank you.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148601

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

[PATCH] D148601: [Clang] Handle Error message to output proper Prefix

[Usman Akinyemi via Phabricator via cfe-commits]

Unique_Usman added a comment.

In D148601#4301977 , @aaron.ballman 
wrote:

> Thank you for working on this! These changes should come with test coverage 
> (we have a handful of verifier tests in `clang/test/Frontend` -- you can add 
> a new test file there), but I don't think a release note is required because 
> this is a fix for internal functionality. The test should cover the simple 
> case of `-verify=something` as well as a more complex test with 
> `-verify=something, something_else`.

Thank you, will work on this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148601

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

[PATCH] D148601: [Clang] Handle Error message to output proper Prefix

[Usman Akinyemi via Phabricator via cfe-commits]

Unique_Usman added a comment.

In D148601#4301977 , @aaron.ballman 
wrote:

> Thank you for working on this! These changes should come with test coverage 
> (we have a handful of verifier tests in `clang/test/Frontend` -- you can add 
> a new test file there), but I don't think a release note is required because 
> this is a fix for internal functionality. The test should cover the simple 
> case of `-verify=something` as well as a more complex test with 
> `-verify=something, something_else`.

Hello, I have been going through the other verfiier test, so basicallhy the new 
test file will test if the error output generated by -verify=something and 
`-verify=something, something_else` is the expected output right?




Comment at: clang/include/clang/Basic/DiagnosticFrontendKinds.td:175-176
 def err_verify_invalid_no_diags : Error<
 "%select{expected|'expected-no-diagnostics'}0 directive cannot follow "
 "%select{'expected-no-diagnostics' directive|other expected directives}0">;
 def err_verify_no_directives : Error<

aaron.ballman wrote:
> Should we be handling this situation at the same time, as it's effectively 
> the same concern? e.g., https://godbolt.org/z/4Mn1rW9oa
Hello, do you meant if we should handle when value is passed to -verify and 
when it is not together?
 
I will say yes, the solution handle the situation whenever the value is passed 
to verify and at the same time was able to handle whenever value is not 
passed(it output the default which is 'expected-no-diagnostics',). 

Do you think otherwise? 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148601

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