Hi,
I'm trying to separate the Support, System, ADT and Config header files
into the support module, per previous plans. However, APFloat.h is not
self-contained:
APFloat.h:105:43: error: llvm/Bitcode/SerializationFwd.h: No such file
or directory
APFloat.h:106:37: error: llvm/CodeGen/ValueTypes.h: No such file or
directory
As you can see, APFloat.h depends on things that are not in the ADT,
Support, or System header files. This, in my mind, constitutes a design
flaw. Can we move the functionality that depends on these header files
elsewhere?
Reid.
The inclusion of SerializationFwd.h can be removed by forward declaring the following two classes in APFloat.h:
Serializer;
Deserializer;
If this doesn't seem like a clean enough solution, there are other ways to potentially decouple APFloat more from the Serialization "library." The tradeoff is that it would require some code rewriting/addition, and could potentially make deserialization of APFloats (as done in the new C frontend) slightly less efficient.
Hi Ted,
> Hi,
>
> I'm trying to separate the Support, System, ADT and Config header
> files
> into the support module, per previous plans. However, APFloat.h is not
> self-contained:
>
> APFloat.h:105:43: error: llvm/Bitcode/SerializationFwd.h: No such file
> or directory
> APFloat.h:106:37: error: llvm/CodeGen/ValueTypes.h: No such file or
> directory
>
> As you can see, APFloat.h depends on things that are not in the ADT,
> Support, or System header files. This, in my mind, constitutes a
> design
> flaw. Can we move the functionality that depends on these header files
> elsewhere?
The inclusion of SerializationFwd.h can be removed by forward
declaring the following two classes in APFloat.h:
Serializer;
Deserializer;
Okay, but I imagine that leaves it still being used in APFloat.cpp which
is also trying to be segregated to the support module.
If this doesn't seem like a clean enough solution, there are other
ways to potentially decouple APFloat more from the Serialization
"library." The tradeoff is that it would require some code rewriting/
addition, and could potentially make deserialization of APFloats (as
done in the new C frontend) slightly less efficient.
I doubt the light performance loss will make any significant difference.
Further, putting all the serialization code in one translation unit may
have some benefit for a) clarity (one place to go instead of all over
the place) and b) might perform better due to cache effects because it
isn't spread all over the executable.
Could you take the necessary steps to decouple APFloat from the
serialization stuff and any other serialized data structures that live
in lib/Support, lib/ADT or lib/System? That would help me get the
separation going. While you're doing that, I'll be working on the
makefile cleanup. Hopefully, soonish, we can have llvm module actually
use the support module.
Thanks,
Reid.
Before I make any changes, I'd like to point out that the serialization code for APFloat is not actually included in APFloat.cpp, and instead is in two source files that are linked into libLLVMBitReader.a and libLLVMBitWriter.a respectively. So the only dependence within APFloat.h on the serialization stuff is the forward declarations of Serializer and Deserializer respectively. Consequently, there shouldn't be any build issues with clients that want to use APFloat but don't want to use the Serialization.
I agree that, when possible, from an interface perspective it is probably cleaner to not have the serialization logic embedded in the LLVM ADT class declarations. I'll look into decoupling the APFloat interface from it's serialization logic on Monday, although the steps I would need to take shouldn't be a blocking issue for you.
Ted
I agree that, when possible, from an interface perspective it is
probably cleaner to not have the serialization logic embedded in the
LLVM ADT class declarations. I'll look into decoupling the APFloat
interface from it's serialization logic on Monday, although the steps
I would need to take shouldn't be a blocking issue for you.
I think the code is fine as it is now.
-Chris