[komh]

Hi/2.

This PR is to add OS/2 kLIBC support.

Review, please...

[avih]

I didn't actually try to follow the exact changes, so this is not a review. Just general and random comments.

1. Did you write these commits completely on your own? if not, which tools were used, and do you understand the changes 100% as if you wrote them yourself? The whole 7 commits are added/written in under 20 minutes - according to the commit dates. That's damn fast to add support to a new platform from scratch...
2. Did you test on OS/2 with klibc that they work?
3. It would help if at least the first commit would describe what klibc is, and in what ways it's better than the default OS/2 build, or otherwise the reasons to support it. Also, describe how to build for OS/2 with klibc. Also, describe the supported platforms, because the klibc page mentions Windows NT too, but it looks like the actuall changes at the PR are assuming this would only be used with OS/2. If klibc supports both OS/2 and NT but the PR is only intended for OS/2, then it should be mentioned at the commit message, and maybe also add some info on how to support it on NT too.
4. There's no need for commit messages to list the new or modified files - that info is already obvious at the commit itself.
5. It would help if each commit message included the reason or reasons for the changes in that commit, which is typically the most important info about a commit. E.g. "change foo to bar" is useless - it's not a reason, it just describes what's visible anyway at the code, but "change foo to bar because otherwise whatever. Note that bar is better than baz in this case because whatever" actually tries to help the reader understand what's going on here.

As an extentsion of the "describe how to build" in an earlier comment, the 1st commit seems to add support at configure.ac, but also adds Makefile.o2k and defines.o2k. Typically only one of those is needed. Why are both here? How did you decide what to put at defines.o2k? all this info should also be part of the commit message.

At the "fix warnings" commit there's this change:

 	size_t length;
-	char *p;
+	char *p = NULL; /* Make compiler happy. */
 	char *qfilename;
 	DECL_GLOB_LIST(list)

Why does it warn about missing init with p but not with qfilename? Both are the same char* type and both are not initalized when defined. One would assume that either both of them generate a warning, or none of them generates a warning. If there's a good reason for this difference, then the commit message should describe it.

[komh]

Hi/2.

I didn't actually try to follow the exact changes, so this is not a review. Just general and random comments.

1. Did you write these commits completely on your own? if not, which tools were used, and do you understand the changes 100% as if you wrote them yourself? The whole 7 commits are added/written in under 20 minutes - according to the commit dates. That's damn fast to add support to a new platform from scratch...

Do you think about AI? ^^ I wrote this PR entirely myself. The reason the time differences between commits are too small is because I commited the patches at the last after writing code.

2. Did you test on OS/2 with klibc that they work?

Sure. I'm running OS/2. Less ported to OS/2 has the problem that scrolling down is not efficiency, To overcome this with the native APIs is the purpose of this PR.

3. It would help if at least the first commit would describe what klibc is, and in what ways it's better than the default OS/2 build, or otherwise the reasons to support it. Also, describe how to build for OS/2 with klibc. Also, describe the supported platforms, because the klibc page mentions Windows NT too, but it looks like the actuall changes at the PR are assuming this would only be used with OS/2. If klibc supports both OS/2 and NT but the PR is only intended for OS/2, then it should be mentioned at the commit message, and maybe also add some info on how to support it on NT too.

I think, Windows NT was the initial purpose. But it has not been realized. kLIBC is only for OS/2. EMX is not maintained and on modern env kLIBC is more proper. I'll write the docs to build with OS/2 kLIBC.

4. There's no need for commit messages to list the new or modified files - that info is already obvious at the commit itself.

It's my mistake.

5. It would help if each commit message included the reason or reasons for the changes in that commit, which is typically the most important info about a commit. E.g. "change foo to bar" is useless - it's not a reason, it just describes what's visible anyway at the code, but "change foo to bar because otherwise whatever. Note that bar is better than baz in this case because whatever" actually tries to help the reader understand what's going on here.

I think, I've fixed.

As an extentsion of the "describe how to build" in an earlier comment, the 1st commit seems to add support at configure.ac, but also adds Makefile.o2k and defines.o2k. Typically only one of those is needed. Why are both here? How did you decide what to put at defines.o2k? all this info should also be part of the commit message.

Because I want to build with both methods, and both works. defines.o2k is the result of configure and I edited some comments.

At the "fix warnings" commit there's this change:

 	size_t length;
-	char *p;
+	char *p = NULL; /* Make compiler happy. */
 	char *qfilename;
 	DECL_GLOB_LIST(list)

Why does it warn about missing init with p but not with qfilename? Both are the same char* type and both are not initalized when defined. One would assume that either both of them generate a warning, or none of them generates a warning. If there's a good reason for this difference, then the commit message should describe it.

Hmm... It's jsut the compiiler's complain. Should I analyze this and explain, too? It's not reasonable requst. Anyway, qfilename is used after setting, but setting p is conditional. This is the difference.

[avih]

Less ported to OS/2 has the problem that scrolling down is not efficiency, To overcome this with the native APIs is the purpose of this PR.

Did you try to fix it in the existing OS/2 build?

Also, again, the reason for this build should be mentioned someplace. If this is the sole reason this platform exists in "less", then users should know it.

By the way, does it build with this klibc too - https://trac.netlabs.org/libc ? because that's the first klibc google result.

[komh]

Less ported to OS/2 has the problem that scrolling down is not efficiency, To overcome this with the native APIs is the purpose of this PR.

Did you try to fix it in the existing OS/2 build?

Yes.

Also, again, the reason for this build should be mentioned someplace. If this is the sole reason this platform exists in "less", then users should know it.

I think, I said this at d81e007.

By the way, does it build with this klibc too - https://trac.netlabs.org/libc ? because that's the first klibc google result.

It's the original version of kLIBC, which has been forked to LIBCn I said before. So it may be built with it but I didn't test with it.

[gwsw]

@komh There are some conflicts between this patch and the current HEAD. They look pretty simple to resolve, but since I don't have an OS/2 system to test on, I'd prefer that you resolve them.

[komh]

I've resolved, thanks!