Segher Boessenkool <seg...@kernel.crashing.org> writes: > On Mon, Jul 24, 2017 at 10:28:06AM +0100, Richard Sandiford wrote: >> > From what I can tell so far it makes things much harder to read. >> > Perhaps that is just because this is all new. >> >> Which parts specifically? E.g. is it mostly the is_a <T> (x, &y) changes? >> Or the as_a <T> (x) changes too? Do you think the FOR_EACH_* iterators >> also make things harder to read? Or is machine_mode->scalar_int_mode >> itself a problem? > > All the as_a <T> (x) etc. looks like cuneiform to me (not just in your > patch); and I cannot read cuneiform :-) > > One day I might understand why we need all this C++ inverted syntax, > needless abstraction, needless generalisation, data hiding and everything > else hiding. Until then, I rant. Sorry. > > The main purpose of abstraction is to make code easier to understand and > to write and change, but with C++ it usually makes it harder it seems :-(
Well, as someone who was/is on the fence about the C++ thing, I definitely sympathise :-) But in this particular case it really isn't (supposed to be) "hey, we're using C++ now, let's add more layers!" abstraction. The same principle would have worked in C and IMO been useful in C. It sounds like you don't necessarily object to SCALAR_INT_TYPE_MODE etc. as asserting forms of TYPE_MODE, so if the syntax is a problem, I think: as_a <scalar_int_mode> (x) => force_scalar_int (x) is_a <scalar_int> (x, &y) => is_scalar_int (x, &y) would be fine too, and shorter to write. Would that be better? Like I say, one advantage of the new wrappers is that they force mode assumptions to be explicit (otherwise the compiler won't build). That caught several real bugs before they had been found and fixed upstream. But that argument might be too weak to support this on its own. The other -- original, and IMO more compelling -- motivation is to make it easier to add variable-sized modes without having to worry about the possibility that scalar or complex modes could be variable-sized (because that would be much more invasive). We could instead have kept everything as machine_mode, made GET_MODE_SIZE always be variable, and forced the result to a constant whenever it's "known" to be constant for some reason. The difficulty with that is that it would be very hard to get right if not testing SVE, and even with SVE you would need just the right input code to trigger the problem. So what we wanted to do was make it "easy" for people to know when variable-sized modes were a concern even if they weren't thinking about or testing SVE. This scalar/not scalar or complex/not complex choices aren't architecture- specific in the same way. With this approach we only needed to force a variable size or offset to a constant in a few places, and those cases were easy to justify from context. Another alternative would have been to add functions like GET_SCALAR_MODE_SIZE that first assert that the mode is scalar and then return a constant size. However, that would have led to a lot more asserts in an enable-checking build and IMO wouldn't have been any more readable than an explicit "is this a scalar?" check followed by the normal GET_MODE_SIZE accessors. Thanks, Richard
