Closed graphitemaster closed 9 years ago
ok ... some are good points ... some others not :
I suggest to avoid ignorance of return values a bit more.
Are you interested to apply aspect-oriented software development? How do you think about to encapsulate error detection and corresponding exception handling as a reusable aspect in your software?
could you point out where return values are ignored please? ... or where does the software depend on undefined behaviour? (or better push a patch if you think there is something wrong)
How do you think about care also for the return value from the call of a function like "pthread_mutex_init"?
Would you like to improve static source code analysis also for your software?
Do you want that these source files should only be safely reused within the implementations of compilers for the programming languages "C" and "C++"?
ok ... added some extra checks to fail gracefully when out of memory conditions happen. Also mutex and attrs initialization is checked ... if you have any hint on how to improve the static analyzer you are more than welcome , so far though I didn't see the clang static analyzer having any problem. I can't understand your last comment. Thanks for your time and let me know if you see anything else worth improving :)
... if you have any hint on how to improve the static analyzer ...
How do you think about to try any more source code analysis tools out? Are there a few update candidates left over?
I can't understand your last comment.
Would you like to avoid to tamper with identifiers in the reserved name space more than it is absolutely necessary?
are you sure that the "a few update candidates" link you posted points to the right place?
also, about tampering with the reserved name space, do you refer to the preprocessor symbols on top of the header files?
maybe I could use a single underscore instead of two ? but did you have any name clash so far?
did you check if it's common practice to use such identifiers (
(I don't know if I like this game of talking only by questions .... anyway .... I'll close this issue, if you see any problem please start a new issue and attach some useful information to fix or improve the code instead of playing the riddle game)
are you sure that the "a few update candidates" link you posted points to the right place?
Yes. - My link pointed to more functions from the POSIX thread programming interface for further considerations.
also, about tampering with the reserved name space, ...
@graphitemaster pointed similar concerns out for the identifier selection. It will be safer to replace some underscores by an unique prefix or delete them.
..., if you see any problem please start a new issue ...
The software improvement can be continued with bug reports like the following.
The use of '' is completely legit and actually is a sort of standard (did you ever grep for "define " in /usr/include/\ ?)
Files in /usr/include are the files provided by the implementation. They are allowed to use the double underscore. You as a library cannot however as the standard explicitly states that anything beginning with double underscore is reserved by the implementation. It's legal for stdio.h for instance to expose a macro like #define __foo int
If your code now uses this identifier, bad things will happen.
Check 7.1.3 Reserved identifiers in the C99 specification which states
All identifiers that begin with an underscore and either an uppercase letter or another underscore are always reserved for any use.
I did not suggest to wrap malloc calls. I suggested checking their return status to see if they return NULL
and to gracefully propagate this error back up the call stack and indicate error to the user of the library. A library that cannot indicate failure of memory allocation is not usable in a robust environment where having a service just unexpectedly crash because of OOM just isn't allowed. Flight control software for instance.
Have you checked the changes from yesterday? I think the library now properly handles OOM conditions and return back an error to the caller. Now, I can understand the point if you are going to use this library in some realtime application but if you run on a generic unix system and you run out of memory chances are that your process will be killed by the system or that your program will just crash next time you call any function from the standard library or not (consider that apart a few functions which return back an error when out of memory, many other will just crash). Now, I'm not saying that you shouldn't care about such out-of-memory cases, but I think you should handle certain conditions only in contexts where it makes sense (for instance advising to always check the return value of pthread_mutex_lock() doesn't make any sense in 99% of contexts). I understand that such a library is pretty generic so I shouldn't have assumed anything and just let the caller make the decision ... hence the recent improvements thanks to your suggestions. But next time a simple patch instead of a series of allusive comments and references to guidelines and conventions would be better and more appreciated. In the end if you don't like this library on you don't trust the code, you can just not use it ;) If instead you are going to use it, please try being more constructive in the feedbacks and provide some code.
... for instance advising to always check the return value of pthread_mutex_lock() doesn't make any sense in 99% of contexts ...
I recommend to prepare the source code of this basic data structure library also for the consistent handling of exceptional situations.
But next time a simple patch instead of a series of allusive comments and references to guidelines and conventions would be better and more appreciated.
It is occasionally more convenient when contributions are provided in the source code language directly. But software evolution will need some consensus before proposed changes will be accepted. There are further design options available to choose the preferred solution.
I understand that patches are more helpful than a series of elusive comments. However some of us are prevented from providing actual working source code due to non compete clauses preventing us from achieving that. Consider feedback the only choice in that circumstance.
@graphitemaster if you're using this in a commercial project, it might be nice to provide @xant some contact work to fix these issues.
Non competition caluses and non disclosure agreements are pretty common in the software development industry. But unless your business is about building libraries for basic datastructures and standard algorithms, I don't see how contributing with fixes/improvements to such an opensource component might fall into those caluses. And in general for any kind of business, if you end up employing some opensource component, it would be nice to contribute back with actual patches or at least with a clear and explanatory feedback. Also being nice and polite would help, even if this is not strictly necessary.
Some quick initial review of libhl turned up some issues with it.
__USE_UNIX98
is a non-standard feature-test macro which should be replaced with_POSIX_C_SOURCE
.__foo
. These identifiers are reserved by the implementation (compiler and libc.) perhaps using anhl_
prefix would be better. This is also seen in headers a lot too;__BINOMIAL_HEAP_H__
for instance.extern "C"
in headers for__cplusplus
This makes it hard to actually use the library for C++ as the symbol names can be mangled, thus not finding them at link stage.#pragma pack(push, ...)
is seen a lot. I'm not sure why you're packing these structures, for doing so can only lead to access violations on systems where the read/write may be unaligned and therefor illegal (PPC for instance.)NULL
when callingfree
. This is just redundant asfree
is required to do this check. Doing this only leads to false-positives when running the code through static-analysis tools like coverity.get_entry_position
needs to hold a lock for a new list node can be inserted into the list in another thread causing the position to be wrong. This can cause a whole slew of problems.malloc
. It can fail, without checking and handling this case, this library is not usable for robust systems.