ymandel marked 2 inline comments as done.
ymandel added inline comments.
================
Comment at: clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.cpp:46
+
+ StringRef Message = "no explanation";
+ if (Case.Explanation) {
----------------
ilya-biryukov wrote:
> ymandel wrote:
> > ilya-biryukov wrote:
> > > ymandel wrote:
> > > > ilya-biryukov wrote:
> > > > > The users will see this for every case without explanation, right?
> > > > > I'd expect the rules without explanation to be somewhat common, maybe
> > > > > don't show any message at all in that case?
> > > > There's no option to call `diag()` without a message. We could pass an
> > > > empty string , but that may be confusing given the way the message is
> > > > concatenated here:
> > > > https://github.com/llvm/llvm-project/blob/master/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp#L204
> > > >
> > > > So, no matter what, there will be some message to go w/ the diagnostic.
> > > > I figure that being explicit about the lack of explanation is better
> > > > than an empty string, but don't feel strongly about this.
> > > Ah, so all the options are bad. I can see why you had this design in
> > > transformers in the first place.
> > > I wonder if we should check the explanations are always set for rewrite
> > > rules inside the clang-tidy transformation?
> > > Ah, so all the options are bad. I can see why you had this design in
> > > transformers in the first place.
> > Heh. indeed.
> >
> > > I wonder if we should check the explanations are always set for rewrite
> > > rules inside the clang-tidy transformation?> Quoted Text
> >
> > I would have thought so, but AFAIK, most folks who write one-off
> > transformations use clang-tidy, rather than writing a standalone tool. They
> > just ignore the diagnostics, i gather. Transformer may shift that somewhat
> > if we improve the experience of writing a (throwaway) standalone tool, but
> > for the time being I think we can't assume that.
> >
> We should focus on minimizing maintenance cost for long-term fixes rather
> than one-off transformations. The cost of passing an empty string to a
> required explanation field for one-off transformations is rather small and
> falls into the hands of a person writing the check, the cost of constantly
> finding and fixing the long-lived upstream checks that don't have explanation
> (leading to bad user experience) is high and will probably fall into the
> hands of `clang-tidy` maintainers in addition to people writing the checks.
>
> FWIW, having a new API of top of plain transformers should be better than
> `clang-tidy` for those writing one-off transformations in the long run. I
> don't think `clang-tidy` was ever designed to cover to cover those use-cases,
> even though today it seems to be the fastest way to do those.
Agreed on all points. I'll send a followup that enforces that constraint.
Repository:
rL LLVM
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D61386/new/
https://reviews.llvm.org/D61386
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
