Worked around some stuff to make it compile in Ubuntu 12.10#2
Open
stendardo wants to merge 2 commits intoDingTo:blender-fixesfrom
stendardo:blender-fixes
Open
Worked around some stuff to make it compile in Ubuntu 12.10#2stendardo wants to merge 2 commits intoDingTo:blender-fixesfrom stendardo:blender-fixes
stendardo wants to merge 2 commits intoDingTo:blender-fixesfrom
stendardo:blender-fixes
Conversation
Makefile. If set, it will set the cmake variable Boost_NO_SYSTEM_PATHS to true, meaning it will ignore all boost installations outside the boost root.
…ns of GCC due to narrowing conversions (which are illegal in C++11). It's IMHO not a good idea to add "-Werror" by default in any public code anyone might be interested in downloading while not being a developer of that particular code.(it should be added as a local variable in the environment of its developers).
Collaborator
|
The BOOST_NO_SYSTEM_PATHS is fine with me, but removing -Werror is not ideal. Our intention is to merge our buildfixes to OSL and I'm not sure just deleting that flag would be acceptable for them. Maybe there should be a cmake option to disable it? |
Author
|
Or a CMake option to enable it would be better (or at least fix the -Wnarrowing errors in the code). Brecht Van Lommel notifications@github.com wrote:
Sent from my Android phone with K-9 Mail. Please excuse my brevity. |
mont29
pushed a commit
to mont29/OpenShadingLanguage
that referenced
this pull request
Jul 25, 2014
There are two ways to expose just an abstract class API from a library, while completely hiding all internals of the class (and in particular, data internals). 1. Subclassing: Make the Foo public API be an abstract class, with only pure virtual methods, and no data. Internal to the library, subclass the parent as 'class FooImpl : public Foo', fully elaborating the methods. This implementation class is hidden from the client app, so you'll need create() and destroy() methods. 2. PIMPL idiom: Make the Foo public API a simple concrete class with no virtual methods, but containing a private FooImpl *m_impl (where FooImpl is a forward-declared class, so not exposed). The ctr/dtr of the exposed class will internally create/destroy a FooImpl, and all the methods of Foo::method() simply call m_impl->method(). This means that the FooImpl also now no longer needs any virtual methods. The cost of these are roughly the same -- DingTo#1 needs to look up methods in the vtbl every time, whereas DingTo#2 needs to make an extra wrapping function call every time (though in theory, a good optimizing compiler with LTO should be able to make this essentially free). If it's important for the client app to be able to completely substitute their own custom implementation while following the same API (i.e., somebody needs to subclass Foo to specialize it), then DingTo#1 is the only way to go. An example of this is RendererServices -- the whole point of RS is to allow a renderer implementation to subclass it in order to provide all the renderer-specific bits of functionality. However, if the library is providing the one and only possible implementation, i.e., no subclassing is ever desired/necessary, and the whole exercise is simply to hide the implementation details behind an abstrat API, then DingTo#2 has a distinct advantage: because there are no virtual methods, Foo can have new (non-virtual!) public methods added, and FooImpl can change arbitrarily, without breaking backward compatibility in linkage with apps! In other words, DingTo#2 allows substantially more change to internals, and adding publicly exposed features, without violating promises that are usually made about compatibility for "release branches" of a product. (Whereas, the DingTo#1 approach dare not change anything about the API, for fear of altering the layout of the vtbl.) So, therefore, this patch "de-virtualizes" the main two public interfaces for OSL, OSLCompiler and ShadingSystem, switching their API abstraction strategy from approach DingTo#1 to approach DingTo#2, and thus serves as powerful insurance as we move forward, allowing much more freedom in the range of changes we make may to internals within release branches without breaking compatibility.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
These commits consist in a work-around in boost library lookup (a CMake issue) as well as the removal of -Werror (which makes compilation in Ubuntu 12.10 fail).