[llvm] r246367 - [Triple] Stop abusing a class to have only static methods and just use

Tue Sep 1 01:38:34 PDT 2015

Sorry for the delay responding, I've been ill for the last day and am quite
behind on a number of things.

Rather than responding in-line, I'm going to try to summarize the issues
because I think there are at least three separate issues here and I want to
address them separately. I'm not sure I can organize my thoughts
effectively by trying to respond in-line. Hopefully this summary will be a
good basis for continuing to discuss this, but if I've missed anything,
please point it out, it wasn't intended.

First, yes, I landed this patch without pre-commit review. I want to lay
out the reasons why, as I think that was justified.

1) I was landing patches to code which I *am* actually very familiar with.
I've worked extensively on the Triple code, Clang's driver code (which is
where much of this code came from originally), and generally on LLVM's
support and ADT libraries. I have worked with post-commit review on very
many parts of them.

2) My patches were applying very widely discussed and reasonably accepted
design patterns within the LLVM codebase and community. This isn't a new or
radical idea, etc. If it were, that would substantially change things.

3) My patches were carefully preserving the existing functionality of the
code. I made extremely certain that this would not regress functionality
that people could be depending on from the existing code.

This is why I felt that it was appropriate to commit without pre-commit
review. I stand by this decision. I'm happy for some of the other long-term
maintainers of this part of LLVM and Clang (because the code really is
clearly intertwined with the Clang driver) to point out why they disagree,
but I would want some specific concerns raised in order for it to be clear
how to make such decisions in the future.

But of course, post-commit review is always an important thing! =D That
brings us to the second issue: should this code use a namespace or a class
with static methods.

I remember this particular pattern coming up a few times in the past and
the discussions seemed clearly to converge on this being an anti-pattern,
and strongly preferring a namespace rather than static methods on a class.
I generally think that is the correct design pattern. If there is strong
consensus from others that in fact we should use classes, we can change
course there a bit, but it would be a significant change IMO.

However, perhaps there is a technical reason why this *needs* to be a class
with static methods in order to enable the tablegen step that you've
referenced a few times? I'm guessing at this based on your comments. I
don't currently understand what the technical requirement would be, perhaps
you could explain that?

If there is indeed a technical reason for doing things this way, that
should also go into the comments so that people who encounter the code
understand the context that necessitates an atypical design. I'm happy to
help with this once the technical issues are clear.

The third issue I see is best discussed on the other review thread, and
that is the issue of using a .def file and macros in order to effectively
reduce the need for dynamic 'strlen' calls and the usage of C-strings in
the interfaces here. I'll keep discussion of those issues on that thread,
but will try to focus in this thread solely on the first two issues I've
outlined.

Anything else that I'm missing?

Thanks, and again I'm sorry it took so long for me to reply here. I'm not
ignoring anything, just have been sleeping and getting healthy again.
-Chandler

On Sun, Aug 30, 2015 at 3:47 AM Renato Golin via llvm-commits <
llvm-commits at lists.llvm.org> wrote:

> On 30 August 2015 at 11:20, Chandler Carruth <chandlerc at gmail.com> wrote:
> > This is a very simple change, and no one seemed actively modifying it,
> so I
> > don't think there is a problem here...
>
> As I said on the other thread, small changes, slow paced.
>
>
> > Even if there are some future plans, we shouldn't design the code in a
> way
> > that makes no sense today because we think (without evidence) that it
> will
> > make more sense in the future. YAGNI, or insert your software design
> > aphorism here.
>
> As I said before, this has been discussed openly on the list. It's not
> because you haven't participated that there is no evidence.
>
>
>
> > This code, today, is doing procedural text parsing, and as such should
> just
> > be using functions in a name space.
>
> Maybe.
>
>
> >If and when some future design is
> > actually proposed, I think that would be the time to change it.
>
> The future design was proposed, and is being implemented as part of
> this is the TargetTuple design, which the TargetParser is waiting on,
> but Eric is blocking the review for the last months, so we couldn't
> progress as fast as we wanted.
>
>
>
> > I don't think that is the correct approach. This patch in particular I
> feel
> > *extremely* confident is a strictly positive step forward.
>
> I don't. What do we do now?
>
>
> > Now if you want to remove *all* of this code until that better state is
> > identified, that makes more sense but I think that ship has sailed. That
> > would be perceived as an acceptable regression. The only path forward I
> see
> > here is to make the code that we have today be reasonable, and ask you
> and
> > others to try to come up with a better design long term. I think these
> > patches are reasonable steps in the first goal.
>
> Shouldn't we have discussed this *before* the patches?
>
> Is it ok for me to start changing other people's areas at will just
> because "I'm extremely confident" that I'm right?
>
> Wouldn't you have the right to revert my patches if that would break
> your line of work?
>
> I'm feeling a big heavy hand here, and I'm not liking it much. Or is
> this the new state of LLVM and we (minor contributors) should just
> accept and move along?
>
> cheers,
> -renato
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150901/4ff2bed9/attachment-0001.html>