Add header with low level I/O helpers.

[de-vri-es]

This PR adds a header with some low level I/O wrappers for Windows, Unix and OSX compatibility, as discussed in #2325.

Most importantly, it should fix the use of ::posix_fallocate for OSX.

I didn't compile yet, as compiling PCL takes quite some time. But feedback is already welcome.

Note that there are a few more modules that have some #define pcl_open ... and similar, but right now I added the header to the IO module. Maybe the header should move to the core module so all modules can use it?

[taketwo]

Thanks for taking care of this!

I'm not particularly fond calling this file "helpers", sounds too generic. What we have inside are OS abstraction functions, so perhaps a name along the lines of "os_abstraction" or "system_abstraction" would be descriptive and to the point. Other ideas are welcome though. @SergioRAgostinho

Another thing is, every file in PCL is copyrighted by OpenPerception. Generally, we allow other companies to be listed in the copyright (e.g. here), but that makes sense when a major contributing (i.e. a new algorithm implementation) was made by their employee. In the case of this PR it's mostly about moving the code around, so I don't think it's fair to extend the copyright.

Note that there are a few more modules that have some #define pcl_open ... and similar, but right now I added the header to the IO module. Maybe the header should move to the core module so all modules can use it?

I'd leave the file in the IO for now. We may examine those other classes at some later point and consider if they actually need the macros at all or whether they can be made dependent on the IO module.

[de-vri-es]

Thanks for taking care of this!

I'm not particularly fond calling this file "helpers", sounds too generic. What we have inside are OS abstraction functions, so perhaps a name along the lines of "os_abstraction" or "system_abstraction" would be descriptive and to the point. Other ideas are welcome though. @SergioRAgostinho

Well, they only abstract low level I/O functions. os_abstraction is claiming a bit much. How about low_level_io.h, or raw_io.h or something along those lines?

Another thing is, every file in PCL is copyrighted by OpenPerception. Generally, we allow other companies to be listed in the copyright (e.g. here), but that makes sense when a major contributing (i.e. a new algorithm implementation) was made by their employee. In the case of this PR it's mostly about moving the code around, so I don't think it's fair to extend the copyright.

I agree that this is a pretty minor change, but it's also not simply moving around code. The originals were macros, these are inlinable functions. The copyright notice in source code is mainly an informational statement, not a legal implication. Copyright can not be transferred without signing a legal document in most countries, and I'm not authorized to sign over copyright for my employer.

I'm fine with adding more copyright statements. Especially the #ifdefs are pretty much copied from existing code (although slightly modified). But I can't simply drop the copyright statement for my employer without discussing that.

I'd leave the file in the IO for now. We may examine those other classes at some later point and consider if they actually need the macros at all or whether they can be made dependent on the IO module.

The only other place where pcl_open is used (seemed like a logical candidate for grepping) is in recognition/include/pcl/recognition/impl/linemod/line_rgbd.hpp. It would be nice to fix that up too before merging.

Maybe it could simply be switched to using the standard std::fopen or std::ifstream? The FD isn't being memory mapped, so I'm not sure if there is a real advantage to using raw IO here.

[taketwo]

There is a problem with namespacing, see Travis log.

I think we should either strip the pcl_ prefix and have these functions in the pcl::io:: namespace, or keep the pcl_xxx names and leave them in the global namespace.

[de-vri-es]

There is a problem with namespacing, see Travis log.

I think we should either strip the pcl_ prefix and have these functions in the pcl::io:: namespace, or keep the pcl_xxx names and leave them in the global namespace.

Yeah, I saw. Probably fixed now. I'm not so sure about removing the pcl_ prefix. That would give them the exact same name as the original functions, which means it depends on the current namespace which one will be chosen.

Of course you can still explicitly call the original functions with for ::open, but it could be confusing to see a call to open(...) with the exact same signature that isn't exactly ::open. If anything breaks for some reason, it is nice if the the wrapper function isn't hidden from whoever has to debug it.

Putting things in the global namespace also isn't very nice though. How about renaming to raw_open, raw_close, etc? That also emphasises the fact that they bypass libc.

[taketwo]

How about low_level_io.h, or raw_io.h or something along those lines?

Generally, I like raw_io.h, however the name can be misinterpreted when looking at the directory listing:

└── io
    ├── ascii_io.h
    ├── obj_io.h
    ├── pcd_io.h
    ├── ply_io.h
    ├── raw_io.h                <----- is this RAW file format I/O?
    ├── vtk_io.h

So I'd probably stick with low_level_io.h.

I'm fine with adding more copyright statements. But I can't simply drop the copyright statement for my employer without discussing that.

If you could list OpenPerception first and then your employer, like we have elsewhere in the library, would it be okay?

Maybe it could simply be switched to using the standard std::fopen or std::ifstream? The FD isn't being memory mapped, so I'm not sure if there is a real advantage to using raw IO here.

I agree, looks like a good candidate to being changed to ifstream functions. One problem though is that the class does not seem to have any unit tests, so we need to be absolutely sure we don't break I/O in some subtle way.

Putting things in the global namespace also isn't very nice though. How about renaming to raw_open, raw_close, etc? That also emphasises the fact that they bypass libc.

OK, I see your point. Let's do that. (raw_falloc does not fit this scheme well though.)

[de-vri-es]

So I'd probably stick with low_level_io.h.

Alrighty, will do.

If you could list OpenPerception first and then your employer, like we have elsewhere in the library, would it be okay?

Yeah, no problem.

I agree, looks like a good candidate to being changed to ifstream functions. One problem though is that the class does not seem to have any unit tests, so we need to be absolutely sure we don't break I/O in some subtle way.

Right, there is a bit of seeking going on, but it should be easy to do the equivalent with std::ifstream. I'll put this bit in a separate commit.

OK, I see your point. Let's do that. (raw_falloc does not fit this scheme well though.)

True, lets see how it looks though :)

Also need to fix more build failures by the looks of it.

[de-vri-es]

Implemented most of the changes

I wasn't sure on the year number to use for the Open Perception copyright statement, so I just went for 2012-, which seemed to be the most common. If I should change it to something else, let me know.

Still want to switch recognition/include/pcl/recognition/impl/linemod/line_rgbd.hpp to std::ifstream.

[taketwo]

There are some compilation issues in Linemod, are you still working on this? Otherwise I'd propose to leave it out and merge the PR (to unbreak PCL for MacOS users).

[de-vri-es]

Good point, I removed the linemod commit. Considering that there are no unit tests, maybe I'll just leave it alone entirely.

[taketwo]

On Windows ssize_t is a typedef. Now that you moved it to the pcl::io namespace, it needs to be qualified. (See AppVeyor log for compilation errors.)

[de-vri-es]

I moved the typedef to the global namespace. Should be fixed now.

[taketwo]

Now off_t seems to be missing. MSDN suggests to use _chsize() variant of this function, whose second parameter is long, so I assume we can typedef long off_t.

[de-vri-es]

I changed it to a long argument now. I actually also didn't use ssize_t in the windows wrappers, but other code is using it in some places.

I figured that the compiler can give better diagnostics when the wrapper functions have the exact same argument/returns types as the underlying function. Otherwise, a known-at-compile-time overflow in a conversion might be hidden by the implicit conversion inside the wrapper functions.

That does mean the signatures aren't identical across windows/unix, but I couldn't think of a reason why that would be a problem.

[de-vri-es]

(I did change to ::_chsize)

[de-vri-es]

Hmm, the failed test loads a PCD file, so the failure could be relevant. Is there any way to get more details from the exception? I don't have a Windows machine myself, so I can't replicate the failure locally.

[taketwo]

This test started to fail after #2247, so nothing to do with your changes.

@matt-deboer this PR addresses the issue you reported two weeks ago. Could you give it a try?

[taketwo]

Superseded by #2354.

[de-vri-es]

Righty. Thanks for the detailed feedback along the way :)