Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Expose macros/functionality for defining interfaces #604

Merged
merged 5 commits into from Jun 22, 2018

Conversation

jsmall-zzz
Copy link
Contributor

Made Result values available in slang.h - make HRESULT compatible where appropriate.
Add macros to support common usage and implementation of interfaces.
Added slang-com-helper.h for helper macros/functionality for interfaces.
Added slang-com-ptr.h provides a strong smart pointer for COM-like interfaces.

Copy link
Contributor

@tangent-vector tangent-vector left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm okay with this, but I am definitely getting a bit nervous about just how much arcane macro stuff somebody has to wade through before getting to the meat of our header. We have certainly crossed the point where we could expect somebody to open up slang.h and just start reading as a way to see what the API is like.

If possible I'd like us to scrutinize what we put into slang.h as much as possible to see what we can get away with (e.g., does the detection macro for "this compiler supports enum class" really need to be in our public API header?).

I think this is important to get the broad strokes of this change in, though, since it can help streamline client code quite a bit. Cleanups to minimize the API surface area that we expose can come later, once they can be informed by knowledge of what clients actually use/need.

@@ -0,0 +1,107 @@
#ifndef SLANG_COM_HELPER_H
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm still leaning toward something like slang-utils.h just so that we don't expose the notion of Slang being a "COM" API in terms of our naming and everything, but this is fine for now.

/* !!!!!!!!!!!!!!!!!!!!!!! C++ helpers !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!*/

#if defined(__cplusplus)
namespace Slang {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like we could eliminate the namespace here, by just not having the two typedefs below, and then having the operator== and != usig SlangUUID instead of Slang::Guid.

That would both streamline the file, and it also avoids a bit of naming mess that I've backed us into: right now we use a capitalized Slang as the namespace name for all our internal implementation code, but the public header exposes any C++-ish stuff under the lowercase slang namespace (which is a bit more of the typical C++ convention, from what I know). I'm okay with this right now because it means our public API and private implementation details have no risk of collision, but it means that in a case like this, the type should really be exposed under the slang namespace.

Not using a namespace at all would resolve that, and in this case shouldn't change the functionality exposed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the namespace usage does seem like a problem - as we will want to use these both internally and externally.
For C++ code using Slang prefixed types seems a little perverse.

Guid guid;
CmpType data[sizeof(Guid) / sizeof(CmpType)];
};
// Type pun - so compiler can 'see' the pun and not break aliasing rules
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be clear, I'm actually pretty sure this isn't actually valid under C++ "strict aliasing" rules, so this could still be undefined behavior. From what I understand, the only foolproof thing would actually be to memcpy from one storage location to another.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe that it is the de facto compiler endorsed type punning mechanism. That it will work on all known compilers with strict aliasing - but may not be strictly by the letter of the standard.

#define SLANG_IUNKNOWN_ADD_REF \
SLANG_NO_THROW uint32_t SLANG_MCALL addRef() \
{ \
return ++m_refCount; \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this call through to a utility function that we add to the Slang API? Something like:

uint32_t spAtomicIncrement(uint32_t* ioVal);
uint32_t spAtomicDecrement(uint32_t* ioVal);

We could use the same thing for the UUID comparison above:

int spCompareUUIDs(SlangUUID const* left, SlangUUID const* right);

This would all add some function-call overhead to these operations, but it would avoid having quite this much implementation in the header (especially if we know some of this implementations stuff needs to be improved iwth things like atomics).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We may want to add support to atomics - although arguably strictly speaking it is not necessary for the addRef/release to be atomic. It can be useful if it is. It is also dependent on the rest of the code base it is being used with. Slang as it stands does not seem to require them to be atomic - it's an implementation detail - that a client of the API can decide on. If interfaces move to more of slang then - there is more of an argument, not least that for 'heavy weight' instances, the overhead is typically minimal.

On spCompareUUIDs - we could add that in that it would make a C implementation more simple.

#define SLANG_COM_PTR_H

#include "slang-defines.h"
#include "slang-result.h"
#include "slang-com-helper.h"

#include <assert.h>

namespace Slang {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The same namespace thing arises here as before: the current convention would be toward this being slang for public API, rather than Slang, but I'm fine with entertaining a switch.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what the right answer is. If we want an internal slang namespace - we should probably just name it 'slang_internal'/'slangInternal'/'SlangInternal'/'Slang::Internal' - or whatever convention we want to use.

There is a school of thought that says namespaces should be lower snake - this seems to be 'inspired' by STL. There is another school of thought that says they should be upper camel - that seems to be inspired more by languages like Java/C#.

Less common seem to be lower camel, or just lower case (!). I think we can rule out just lower camel cos it seems uncommon and less readable. Lower camel again seems unusual - but at least it's more readable than just lower case.

That leaves lower snake and upper camel. The 'advantage' seen with lower camel (other than STL) is that it makes it clear that it's a namespace. The 'advantage' of upper camel (other than similarity to Java/C#) is sort of the opposite - that it sort of unifies them, as typically you don't want or need to know the difference.

So lets call that a tie. Perhaps the tie breaker is what kind of API are you producing. Does it have methods with lower camel (like STL)? Or lower camel (like C#/Java)? Then perhaps it can be argued your namespacing convention should follow the conventions of the thing that you are closest too.

By this argument upper camel matches most closely slangs other naming conventions. That said - this isn't a killer argument, and if you wanted to make it clearer where a namespace was being used lower camel, would be acceptable.

I would also lay bare my preference is where all things being equal for upper camel - for readability, conciseness and continuity reasons.

@jsmall-zzz jsmall-zzz merged commit d0c9571 into shader-slang:master Jun 22, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants