Make preprocess and getIncludePath const functions.

[fruffy]

There is no compelling reason why the preprocessing function and the getIncludePath function should not be the const. I changed the interface such that these functions can be used as const functions. The only thing that doesn't work is `file = "", not clear to me how it is used.

[vlstill]

The whole thing around closeInput is ugly, but that is a different problem. Or maybe just add a destructor to PreprocessorResult that closes the FILE * if necessary?

[fruffy]

The whole thing around closeInput is ugly, but that is a different problem. Or maybe just add a destructor to PreprocessorResult that closes the FILE * if necessary?

The destructor might close the file too early if not handled correctly. I added an explicit function instead.

[ChrisDodd]

A lot of this mess comes from the incompatibility of popen with iostreams.

[vlstill]

The whole thing around closeInput is ugly, but that is a different problem. Or maybe just add a destructor to PreprocessorResult that closes the FILE * if necessary?

The destructor might close the file too early if not handled correctly. I added an explicit function instead.

How could an explicit member function close it after the destructor? That would mean the object is already destructed and the member function can't be called. You might need to define a move operator though (the invalidates the source).

[fruffy]

A lot of this mess comes from the incompatibility of popen with iostreams.

Would be great if we could call into the preprocessors as a library, would solve a bunch of problems.

How could an explicit member function close it after the destructor? That would mean the object is already destructed and the member function can't be called. You might need to define a move operator though (the invalidates the source).

Instead of adding the file handle closing in the destructor I just added a member function that can be called.

[vlstill]

How could an explicit member function close it after the destructor? That would mean the object is already destructed and the member function can't be called. You might need to define a move operator though (the invalidates the source).

Instead of adding the file handle closing in the destructor I just added a member function that can be called.

I can see that. My point is that you can't call a member function after a destructor. That would mean calling a function on a destructed object, which is UB. So a destructor can't be "too early". The only possible problem with destructor is that there might be multiple copies/moved versions of the same PreprocessorResult. The standard solution in this case is to forbid copying and invalidate the moved-from object so that only the last moved-to object actually closes the stream. That is the C++ way (and exception-safe way) of doing this.

[fruffy]

The standard solution in this case is to forbid copying and invalidate the moved-from object so that only the last moved-to object actually closes the stream. That is the C++ way (and exception-safe way) of doing this.

With too early I meant what you described, where you have multiple objects and the file handle gets closed prematurely because one of the copies is destroyed. It seems safer to make this explicit instead of worrying about copy/move semantics.

[ChrisDodd]

The standard solution in this case is to forbid copying and invalidate the moved-from object so that only the last moved-to object actually closes the stream. That is the C++ way (and exception-safe way) of doing this.

With too early I meant what you described, where you have multiple objects and the file handle gets closed prematurely because one of the copies is destroyed. It seems safer to make this explicit instead of worrying about copy/move semantics.

Well, making the object non-copyable will avoid that (and is much simpler)

[fruffy]

Well, making the object non-copyable will avoid that (and is much simpler)

It makes working with the returned object itself a fair bit more involved though. I didn't want to make the return value of the call overly complicated.

[ChrisDodd]

Well, making the object non-copyable will avoid that (and is much simpler)

It makes working with the returned object itself a fair bit more involved though. I didn't want to make the return value of the call overly complicated.

Usually it makes dealing with the returned object much simpler -- you just return by value and it "just works" and you don't need to do anything special. That's the whole point of movable-not-copyable objects.

[fruffy]

Well, making the object non-copyable will avoid that (and is much simpler)

It makes working with the returned object itself a fair bit more involved though. I didn't want to make the return value of the call overly complicated.

Usually it makes dealing with the returned object much simpler -- you just return by value and it "just works" and you don't need to do anything special. That's the whole point of movable-not-copyable objects.

Okay for file handles this does make sense and after rewriting parseInput the code change was not particularly involved. The previous version I had caused a copy which is why deleting the constructor didn't work. This is better indeed.

[vlstill]

Thanks for the change. I suspect the move on the PreprocessorResult is not used, but if it would, it would be wrong.

[vlstill]

This will not work. The default move will do memberwise move, but for primitive types that is just a copy. So we either need to disable the move constructor too (which would probably work, otherwise there is a double-close somewhere now), or explicitly set orig._file = nullptr in the move operator/ctor.

[asl]

Or, maybe, one could use std::unique_ptr<FILE, decltype(&fclose)> This way the owning semantics would be preserved.