[PATCH] D79375: [FileCheck] Make invalid prefix diagnostics more precise
Joel E. Denny via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue May 5 12:26:17 PDT 2020
jdenny marked 2 inline comments as done.
jdenny added inline comments.
================
Comment at: llvm/lib/Support/FileCheck.cpp:1905
+
+static const char *DefaultCheckPrefixes[] = {"CHECK"};
----------------
thopre wrote:
> jdenny wrote:
> > jhenderson wrote:
> > > This is only used in one place. Why not just explicitly provide it there? Alternatively, should this be defined in the top-level bit of FileCheck, so that client libraries can have different default check prefixes?
> > > This is only used in one place. Why not just explicitly provide it there?
> >
> > I changed it. It was that way because I extracted it from D79276, which still uses it in two places.
> >
> > > Alternatively, should this be defined in the top-level bit of FileCheck, so that client libraries can have different default check prefixes?
> >
> > Perhaps so. But I feel like that's an orthogonal issue and belongs in a separate patch. What do you think?
> I don't think we should allow customized default directive until the need arises.
I agree. Let's wait until there's a specific need that cannot be handled by `-check-prefix[es]` or `FileCheckRequest::CheckPrefixes`.
================
Comment at: llvm/lib/Support/FileCheck.cpp:1891-1893
+ errs() << "error: supplied check prefix must start with a letter and "
+ << "contain only alphanumeric characters, hyphens, and "
+ << "underscores: '" << Prefix << "'\n";
----------------
thopre wrote:
> Could you make ValidateCheckPrefix throw a DiagnosticError and print the error here?
That seems ok, but what's the motivation? Is it a preferred style, or is there a technical advantage?
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D79375/new/
https://reviews.llvm.org/D79375
More information about the llvm-commits
mailing list