Preview of contribution / More .ini APIs (nearly completes set of PrivateProfile API) #250
Conversation
…rivateProfile API). Won't compile as it needs hamcrest and junit 4.
| */ | ||
| public static final List<String> getPrivateProfileSection(final String appName, final String fileName) { | ||
| final char buffer[] = new char[32768]; | ||
| if (Kernel32.INSTANCE.GetPrivateProfileSection(appName, buffer, new DWORD(buffer.length), fileName).intValue() == 0) |
There was a problem hiding this comment.
You should rewrite this by making two calls, otherwise this has a hidden limitation of an upper 32K bound. Call it once to get the size, allocate the string and call it again.
MSDN says: the return value specifies the number of characters copied to the buffer, not including the terminating null character. If the buffer is not large enough to contain all the key name and value pairs associated with the named section, the return value is equal to nSize minus two.
There was a problem hiding this comment.
There is no 'hidden' limitation. Actually the limitation is documented in MSDN (http://msdn.microsoft.com/en-us/library/windows/desktop/ms724348(v=vs.85).aspx): ...nSize [in]
The size of the buffer pointed to by the lpReturnedString parameter, in characters. The maximum profile section size is 32,767 characters...
Hence, the above function is perfectly well and nothing is "hidden". :-)
|
Please update the CHANGELOG. I am happy to run the tests when merging this. As far as the lengthy explanation of what/why/how/when you want or don't want to contribute, it's unnecessary. We take what you're willing to give and ask for nothing else. |
|
Thank you for this clarification. I wrote the lenghty explanation since we provided a piece of code which will neither compile nor test, which possibly could be interpreted as being unwilling to provide "good" code etc. I wanted to point out that the actual reason is simply the missing budget for more rewrites. Thanks for fixing the tests! :-) I will modify the CHANGELOG so you can go on then. |
|
+1 move to current JUnit (aside from a very few items identified by Farkas, current code should still compile) On Jul 21, 2013, at 12:25 PM, Markus Karg wrote:
|
|
Hamcrest is a library (and develops to a de-facto standard) which extends unit testing frameworks like JUnit by a fluent assertion API for executing in the unit testing framework of choice (https://code.google.com/p/hamcrest/). It is contained in JUnit since 4.x, but has made significant progress made since, so virtually everybody using Hamcrest is not only using the version found in JUnit but instead uses latest. With Hamcrest you can easily write readable and powerful assertions, for example...: Hamcrest can reduce many tests by far, up to 50% or more with complex assertions. Again, it is part of JUnit anyways, but we mentioned it as we used explicitly the latest version in our code. |
…here maximum numbers are taken from.
|
Why not make this future-proof, call it twice and avoid growing the heap unnecessarily? <-- I meant to say heap, it's a |
|
There is actually no need for such "future proof" improvement, as according to Microsoft, the complete INI API SOLELY exists for backwards compatibility with existing applications. It in fact is marked as deprecated for more than thirteen years meanwhile, and since (at least) Windows 2000 nobody shall use "INI file" operating system objects anymore in any new applications but instead use "registry key" operating system objects (hence: the registry API, not the INI file API). So Microsoft will never expand that 64KiB maximum. See the note found in all INI API docs: "Note: This function is provided only for compatibility with 16-bit applications written for Windows. Applications should store initialization information in the registry." (e. g. here: http://msdn.microsoft.com/en-us/library/windows/desktop/ms724348(v=vs.85).aspx) Besides that, your suggestion of calling twice will not work, as the function does not tell you the actually needed buffer size, as the above mentioned API docs say. There is no way to find out the buffer size (for good reason: because you can simply use a static 64KiB size due to Microsoft's mention documentation). Besides that I think it is not really worth even discussing to spare a size of 64KiB on a win32 machine, which are only temporarily (for few microseconds) allocated, in the days of 4+ GiB entry-level laptops... ;-) |
| @Test | ||
| public final void testGetPrivateProfileSection() throws IOException { | ||
| // given | ||
| final File tmp = File.createTempFile("testGetPrivateProfileSection"(), "ini"); |
There was a problem hiding this comment.
Please replace the last line by
final File tmp = File.createTempFile("testGetPrivateProfileSection", "ini");
then it will compile. Simply a typo when writing the PR without having the ability to test. Remember, test-platform was made able to run without Cygwin AFTER that code was developed...
There was a problem hiding this comment.
Please fix this yourself. You're creating busy work for the maintainers of the project.
|
I checked this out. There're two obvious compile problems, but once fixed there're more. The Eclipse project is broken, it references JUnit3. Switching to JUnit4 that comes with my version of Eclipse doesn't quite work, it seems that the version of hamcrest with that is not 1.3 which you tried to include here. Manually referencing junit.jar and hamcrest-core-1.3.jar works. Those should live in lib/test instead of lib, and there're various places to adjust paths.I started making some of these changes in https://github.com/dblock/jna/tree/QUIPSY-MoreInitializationFileAPIs, but maybe there should be less work to do this (like upgrading the minimum requirement for Eclipse that has JUnit 4.11). So back to you. This PR needs work. |
|
I am uncertain what you want me to do. I told you in the Initial comment that the code Needs latest JUnit and latest hamcrest. You said you take what I give and you'll do the rest. So what exactly you expect me to deliver now? Do you want me to learn the complexity of the existing build script to do the JUnit and Hamcrest updates, or do you want me to get rid of latest JUnit and Hamcrest and rewrite the code? Do you want me to fix the Eclipse project? Some months back I asked about using that Eclipse project and you said I do not need to care about it. |
|
If you insist on replacing JUnit with a newer version and Hamcrest, then please make it work for everything that's already in-place. I may have said that you shouldn't care about the Eclipse project, but I also assumed the fixes would be trivial, as usual, and they are not. So this now creates half a day of work for me to go and finish the upgrade to JUnit 4.x, which frankly, provides very little value and takes a lot of time. What I'd like is pull requests that don't break existing things. If I were you I would just make a PR with the new APIs on top of existing infrastructure, aka a small and incremental change. |
|
This is exactly what I tried to explain in the discussion before in the form: The code comes from an different, existing project, and that project was done in latest JUnit and Hamcrest and Java 7. If JNA wants our contribution, someone must to the rewrite, as we cannot afford (why should we?) rewriting the complete code. You said I shall open a PR so you can see what I mean. So now you see what I mean. I will rewrite the code to get rid of latest JUnit and Hamcrest, but frankly spoken, this was the last contribution to JNA then, as we have no Budget to invest days into rewrites. I do not insist on anything, this is just the simple truth of the fact that this is not our hobby but we are contributing existing formerly closed source for the sake of this open source project. See the dilemma? |
|
Just leave this PR open and I'll do the rewrite work when I get to it. Cause I enjoy it. I don't mean to start a debate about open-source vs. budgets. I wrote this blog post a while ago on the subject. Also. This. If you want a more utopian perspective. |
|
Copy pasted the mappings and fixed the tests without any external dependency changes in c7dcf61. |
To extend and nearly complete Win32's PrivateProfile API, our company likes to contribute these missing mappings. The code origins from a commercial project and was backported into JNA with respect to your licence. We send this preview, because we have the following problems and like to get your opinion how to go on now:
We're hopeing that you find a wise and fair answer, so we can go on and contribute more of our commercially developed code to the JNA open source project. Please understand that days of backports / rewrites are out of scope of our sponsored budget for the JNA project, but one the other hand, we really would love to give away all that code we developed in the past to the JNA project.
Please also note that more contributions are ready to get previewed, but as they also are done using Junit 4.11 / Hamcrest, and as they depend on the APIs provided in THIS pull request, it makes no sense to contribute them before we received an answer that technically allowes us to technicall provide a mergable pull request.