Fixes issue #371 - outName static cast error in debug builds

[fearn-e]

@AlexHarker
As discussed in #372, here's a new PR with a cleaned up branch.

This fixes the original static cast bug that arose from a CLASS_ATTR_SYM being used for a t_atom type.

Instead of changing the CLASS_ATTR, as the variable is only ever used as a t_atom once and is being converted to a symbol type many times, it is changed to a t_symbol type and now only ever converted to a t_atom once (lines 257-259).

[AlexHarker]

This looks correct to me. @tremblap - can you review please?

[tremblap]

@AlexHarker should we avoid allocation at line 257 and make it part of the struct?

[AlexHarker]

@AlexHarker should we avoid allocation at line 257 and make it part of the struct?

There is no heap allocation in this case - the t_atom is just a local variable allocated on the stack which is pre-allocated for the thread (I think that's when the allocation happens).

Allocating in the object is actually a potential source of less efficiency, because the object requires dereferencing and although it is likely to be in a cache line, the stack definitely is, so no I don't think we should make it part of the struct.

[tremblap]

if @weefuzzy agrees then I'll merge. I've just learnt I was drilled to be paranoid about mem alloc :D

[weefuzzy]

Can someone catch me up on why #372 got closed and this more extensive PR replaced it?

[AlexHarker]

Yes - it's a better fix, because the attribute should never be anything other than a symbol (and the atom was getting set to a symbol manually in the code). Previously max thought the field in the object it was a t_symbol *, and likely set the first part of the t_atom and it just happened to work, because the t_atom was getting set elsewhere, thus setting the type flag.

I see nothing here to worry about, but there were possible concerns about just changing it to CLASS_ATTR_ATOM (like possibly allowing the user to set it to a number).

[weefuzzy]

@AlexHarker I didn't see your comment fearn-e@d410b7e#commitcomment-123632941 for some reason, which would have reduced the mystery.

FWIW, my maintainer-preference is always for the smallest fix needed to close an issue, and any secondary stuff (like whether it would be better to have another type or whatever) to be followed up as separate changes with their own (quite probably lower priority) issues. IOW, I don't doubt that you're right, but the future process of working out why a commit is the way it is will be more befuddling for future-me now.

[weefuzzy]

@tremblap if / when you're happy it works on both platforms, then please merge. As @AlexHarker says, there isn't a worrying allocation issue.

[AlexHarker]

I tend to leave any problems to future-me. It always works out really well.