-
-
Notifications
You must be signed in to change notification settings - Fork 340
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
informal code review of the libconf branch #18
Comments
I have started a review of this branch to intend to rework the branch to make it cleaner to read. I want to:
Based on that, I think some work on libconf should be done:
|
First of all, I think Vasek can precise what he has done for mge-xml and AIX. Have the code been merged (or picked) to master ? as master looks like having similar code ? |
Fred opened a few pull requests for Vasek's work. Some of the commit messages were not very specific, so they were cherry-picked and reworded (rather than merged). AIX compilation fixes: #58 I am not sure about some of the AIX packaging improvements. If you do an interactive rebase onto master, then you probably want to skip d2e1bb0, 704d763, and 72d3fd1 from the mge-xml fixes. Unfortunately, there may be some other merge conflicts if Git doesn't pick up the rename of configure.in to configure.ac. |
Thanks Charles, I will look at them. |
…0828 FTY remerge 20170828 - foundation for lookup functions support in DMF
Some of the warnings on the clang builders prompted me to take a closer look at the libconf branch.
NutStream::status_t NutFile::putData(const std::string & data) has a comment that there is not a FILE* interface for writing arbitrary data. What about fwrite()?
tempnam() should probably be replaced with mkstemp(). Then you won't need the hard-coded /var/tmp variable. (Some systems implement per-user $TMPDIR variables, which eliminate some corner cases and race conditions.)
This expression won't do bit operations: ss << (packed && 0x000000ff) << ".";
Also, you should consider using the standard inet_?to? functions here, especially for the IPv6 address printer.
The text was updated successfully, but these errors were encountered: