[PATCHSET] Make Haiku userland compile and work with Clang

[js]

This patch set makes it possible to compile the Haiku userland with Clang and successfully boot and use it.

[stippi]

For patch 0001, is there a reason the SetConfigVar line had to be repeated in those Jamfiles? Can't it be done somewhere central? It looks wrong to me to repeat the definition everywhere.

[stippi]

For 0003, I don't remember the details, but should I be credited in the BStackOrHeapArray? :-P

[stippi]

All the rest of the patches look fine to me. I only think 0001 could be more elegant.

[js]

For 0001: As stated in the commit message, neither I nor PulkoMandy found a better way. I'm really looking for a better way, as this is really ugly. If someone knows one, please let me know!

For 0003: You suggested to create such a class, but I wrote it :P.

[stippi]

Ah, sorry, missed the explanation. Isn't the build system already working with some global variables that are visible from all Jamfiles? We not add this where global variables are declared? Like even from the configure script. (configure --with-clang=...)

[js]

Well, it should not be global, only for the glibc directory and its subdirectories. That was the problem ;).

[stippi]

I am applying and testing everything except 0001. Hoping Ingo will comment, as he can most likely think of the proper solution.

[stippi]

Also, it would already help to just declare the parameters passed to SetConfigVar somewhere global. Like "GLIBC_CLANG_CONFIG_VARS = ..." somewhere global and then in the individual Jamfiles, pass that to SetConfigVar. That way, one could at least change what is passed in one place.

[js]

Well, that would be yet another workaround. But i'd rather do it right, which means recursively. I'm sure jam can do that and I just haven't figured out how ;).

[stippi]

I have pushed everything except the first patch, giving up ownership of the ticket. Oh, I realize my "accepting" the ticket didn't work, since I didn't retry after the message that the ticket was changed before I commented... :-)

[js]

I found a solution for the Jam problem now. I remembered that you could set flags for directories in UserBuildConfig and found it there. I used the same method now. I attached an updated 0001.

I also added 0011 which fixes a ++ that was accidentally added. Thanks to PulkoMandy for noticing this in the diff!

[kallisti5]

0001 and 0011 applied in hrev46660 as per irc.

[kallisti5]

marking closed as per js in irc

[stippi]

There are some problems with BStackOrHeapArray. Re-opening. In any Tracker window, in the size column, the strings are corrupted. I am not yet sure what can cause this, but the string width stuff in app_server is one thing that was touched by the BStackOrHeapArray changes. In particular, it turns out I do hate the Type* operator(), since it makes it non-intuitive for someone like me what actually happens. And it seems that something non-obvious happens with those casts in app_server.

[diver]

I think the corrupted strings have been fixed in hrev46660.

[js]

String corruption does indeed sound like the ++ that was accidentally added in glibc by 0006. Can you try hrev46660?

About the Type* operator(): This basically makes sure that you can use it like a normal array, i.e. it gets automatically converted to a pointer, just like a normal array. The idea was that you can easily switch code to BStackOrHeapArray. The only thing that of course will break is sizeof(). But sizeof on an array isn't a good idea anyway, as just switching that array to a pointer will also break it.

Not having that operator would mean code needs to be changed in ugly ways. E.g. if you want to pass a pointer to the data of the array (which seems to be the most common operation), you'd need to change it to &foo[0] instead of foo.

[stippi]

Oh, I thought I was running the latest version. Probably not. Sorry about the noise!

[js]

So it does work now and the ticket can be closed? ;)

[stippi]

Replying to js:

About the Type* operator(): This basically makes sure that you can use it like a normal array, i.e. it gets automatically converted to a pointer, just like a normal array. The idea was that you can easily switch code to BStackOrHeapArray. The only thing that of course will break is sizeof(). But sizeof on an array isn't a good idea anyway, as just switching that array to a pointer will also break it.

I understand that much. What wasn't clear to me is how the compiler knew what to do in cases such as these:

BStackOrHeapArray<int32, 64> someInts(arraySize);

SomeFunctionExpectingSizeTPointer((size_t*)&someInts[arrayIndex]);

But Ingo told me that the compiler has to try to convert the object to a pointer first, before it can access it as an array, and since the Type* operator() is the only way to do that, it does indeed do the right thing. Still, it can be non-obvious what happens there, at least it has been for me. What is or isn't ugly code, is really debatable. For example, I would not have considered this ugly code, and this was my original suggestion all the time ago:

BStackOrHeapArray<int32, 64> someIntsArray(arraySize); int32* someInts = someIntsArray.Data();

SomeFunctionExpectingSizeTPointer((size_t*)&someInts[arrayIndex]);

This is just one more line per declaration, but removes BStackOrHeapArray completely from what you need to know about the code that follows. Or in another words, if you could understand the code before the change, you can still understand it just fine, no extra knowledge needed.

(I know that one should not cast an int32 to size_t, because it breaks on x86_64... there are TODOs in app_server about this.)

[js]

Yes, but that means you need to add that one line of code everywhere where you change it. So it's just more boilerplate and makes diffs to switch code over to BStackOrHeapArray bigger. I fail to see a gain there. Maybe I would if you could provide an example for a situation where it's non-obvious?

[stippi]

Can confirm that the string corruptions are gone.

[stippi]

I gave you the example in comment:21 and explained what would be gained for me. Please re-read, I don't feel like repeating myself. Now, if that example is obvious to you, or removing having to know about BStackOrHeapArray to understand the code which uses it, is not a gain for you, that is a different thing. We just have to accept that we disagree there.

[js]

Ok, I thought you meant another example, because the one you gave was pretty obvious to me. The [] operator comes before the & operator (operator precedence). There is no explicit [] operator, so it falls back to the C behaviour, where [] is just *(pointer + offset). As it needs a pointer, it uses the Type* operator(). Then it can evaluate someInts[arrayIndex] and takes the pointer of that and cast it. It all boils down to operator precedence. Would it be more obvious to you if the code were "(size_t*)(&(someInts[arrayIndex]))"? Because that's basically how the compiler treats it due to operator precedence.

[stippi]

No, problem, but you still don't quite understand what I mean. You don't need to explain to me about operator precedence, although that stuff is generally more fuzzy to me than other areas. However, I wrote that code, so as far as that is concerned, I understand it.

I am talking about one new thing introduced by BStackOrHeapArray that one needs to understand here, which is that before the compiler can use the array operator, it needs to apply a pointer cast. If one knows that, it becomes clear why the compiler knows how to do the right thing in my example, and why a [] operator is not needed in BStackOrHeapArray.

So the gain for me is not having to know an additional thing. Obviuosly I knew that my solution would require one more line to be written, I even pointed it out in that comment. So you don't need to remind me of that, but you need to ask yourself why I am Ok with it. The answer is simply that it is slightly more important to me not needing to know one more thing about the code than having to write one more line. That's all. I don't think "ugly" has anything to do with it. That is like thinking code needs to be short above anything else, which is certainly not true as simple as that.

[js]

Oh, I see what you mean. So what you is that it might be confusing that Type* operator() is used for [], because it might not be obvious enough. What about adding an operator[] then? Then it's clear, isn't it? It would be useless code, but make it clear. Or we could add a comment explaining it. I think both would make it equally clear, but the first would introduce unnecessary code overhead.

[stippi]

When I still thought there was something broken, I actually tried to add the operator[]. If memory serves, I tried adding this:

Type& operator[] (int const& index) {

return fData[index];

}

const Type& operator[] (int const& index) const {

return fData[index];

}

But then I got compiler errors about some ambitious conversions, specifically in that archiving code, but I don't know if there would have been more. So maybe the best option could be the comment. Or maybe just do nothing, I just tried to explain what benefit I would personally see with not using any operator at all and just having one more line to type. But I may not be a good measure for these things. Maybe it is obvious to most other people why the pointer operator is enough. After all, when using the class, one should look in the header...

BTW, when googling this stuff, I also read that Type* operator() is usually declared const. It makes sense to me. Do you think that should be ammended in the header?

[js]

The comment is definitely something we should add. It confused you, so it might confuse others as well.

const Type* operator() has a problem: It would mean the data the pointer points to is read-only. But if you want to modify the data in the array, this would be a problem.

[pulkomandy]

I think stippi meant operator Type*() const ?

You have to be careful with const, it can mean a lot of things :)

[js]

Ah, I see. I think that const might not be correct, as we return a non-const pointer and thus the internal state could be changed. A const Type* operator() const OTOH could work.