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.

Sign up for GitHub

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

Jump to bottom

Expose macros/functionality for defining interfaces #604

Merged
merged 5 commits into from Jun 22, 2018
Merged

Expose macros/functionality for defining interfaces #604

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3767b5e
Added Result definitions to the slang.h
jsmall-zzz Jun 22, 2018
19c6896
Removed slang-result.h and added slang-com-helper.h
jsmall-zzz Jun 22, 2018
71d95ad
Move slang-com-ptr.h to be publically available.
jsmall-zzz Jun 22, 2018
476411d
Add SLANG_IUNKNOWN macros to simplify implementing interfaces.
jsmall-zzz Jun 22, 2018
2c98a45
Removed slang-defines.h added outstanding defines to slang.h
jsmall-zzz Jun 22, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
107 changes: 107 additions & 0 deletions slang-com-helper.h
View file
Open in desktop
@@ -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.

#define SLANG_COM_HELPER_H

/** \file slang-com-helper.h
*/

#include "slang.h"

/* !!!!!!!!!!!!!!!!!!!!! Macros to help checking SlangResult !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!*/

/*! Set SLANG_HANDLE_RESULT_FAIL(x) to code to be executed whenever an error occurs, and is detected by one of the macros */
#ifndef SLANG_HANDLE_RESULT_FAIL
# define SLANG_HANDLE_RESULT_FAIL(x)
#endif

//! Helper macro, that makes it easy to add result checking to calls in functions/methods that themselves return Result.
#define SLANG_RETURN_ON_FAIL(x) { SlangResult _res = (x); if (SLANG_FAILED(_res)) { SLANG_HANDLE_RESULT_FAIL(_res); return _res; } }
//! Helper macro that can be used to test the return value from a call, and will return in a void method/function
#define SLANG_RETURN_VOID_ON_FAIL(x) { SlangResult _res = (x); if (SLANG_FAILED(_res)) { SLANG_HANDLE_RESULT_FAIL(_res); return; } }
//! Helper macro that will return false on failure.
#define SLANG_RETURN_FALSE_ON_FAIL(x) { SlangResult _res = (x); if (SLANG_FAILED(_res)) { SLANG_HANDLE_RESULT_FAIL(_res); return false; } }
//! Helper macro that will return nullptr on failure.
#define SLANG_RETURN_NULL_ON_FAIL(x) { SlangResult _res = (x); if (SLANG_FAILED(_res)) { SLANG_HANDLE_RESULT_FAIL(_res); return nullptr; } }

//! Helper macro that will assert if the return code from a call is failure, also returns the failure.
#define SLANG_ASSERT_ON_FAIL(x) { SlangResult _res = (x); if (SLANG_FAILED(_res)) { assert(false); return _res; } }
//! Helper macro that will assert if the result from a call is a failure, also returns.
#define SLANG_ASSERT_VOID_ON_FAIL(x) { SlangResult _res = (x); if (SLANG_FAILED(_res)) { assert(false); return; } }

/* !!!!!!!!!!!!!!!!!!!!!!! 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.


// Alias SlangResult to Slang::Result
typedef SlangResult Result;
// Alias SlangUUID to Slang::Guid
typedef SlangUUID Guid;

SLANG_FORCE_INLINE bool operator==(const Guid& aIn, const Guid& bIn)
{
// Use the largest type the honors the alignment of Guid
typedef uint32_t CmpType;
union GuidCompare
{
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.

const CmpType* a = reinterpret_cast<const GuidCompare&>(aIn).data;
const CmpType* b = reinterpret_cast<const GuidCompare&>(bIn).data;
// Make the guid comparison a single branch, by not using short circuit
return ((a[0] ^ b[0]) | (a[1] ^ b[1]) | (a[2] ^ b[2]) | (a[3] ^ b[3])) == 0;
}

SLANG_FORCE_INLINE bool operator!=(const Guid& a, const Guid& b)
{
return !(a == b);
}


/* !!!!!!!! Macros to simplify implementing COM interfaces !!!!!!!!!!!!!!!!!!!!!!!!!!!! */

/* Assumes underlying implementation has a member m_refCount that is initialized to 0 and can have ++ and -- operate on it.
For SLANG_IUNKNOWN_QUERY_INTERFACE to work - must have a method 'getInterface' that returns valid pointers for the Guid, or nullptr
if not found. */

#define SLANG_IUNKNOWN_QUERY_INTERFACE \
SLANG_NO_THROW SlangResult SLANG_MCALL queryInterface(SlangUUID const& uuid, void** outObject) \
{ \
ISlangUnknown* intf = getInterface(uuid); \
if (intf) \
{ \
addRef(); \
*outObject = intf; \
return SLANG_OK;\
} \
return SLANG_E_NO_INTERFACE;\
}

#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_IUNKNOWN_RELEASE \
SLANG_NO_THROW uint32_t SLANG_MCALL release() \
{ \
--m_refCount; \
if (m_refCount == 0) \
{ \
delete this; \
return 0; \
} \
return m_refCount; \
} \

#define SLANG_IUNKNOWN_ALL \
SLANG_IUNKNOWN_QUERY_INTERFACE \
SLANG_IUNKNOWN_ADD_REF \
SLANG_IUNKNOWN_RELEASE

} // namespace Slang
#endif // defined(__cplusplus)

#endif
60 changes: 5 additions & 55 deletions source/core/slang-com-ptr.h → slang-com-ptr.h
View file
Open in desktop
@@ -1,16 +1,15 @@
#ifndef SLANG_COM_PTR_H
#ifndef SLANG_COM_PTR_H
#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.


/*! \brief ComPtr is a simple smart pointer that manages types which implement COM based interfaces.
\details A class that implements a COM, must derive from the IUnknown interface or a type that matches
it's layout exactly (such as IForwardUnknown). Trying to use this template with a class that doesn't follow
it's layout exactly (such as ISlangUnknown). Trying to use this template with a class that doesn't follow
these rules, will lead to undefined behavior.
This is a 'strong' pointer type, and will AddRef when a non null pointer is set and Release when the pointer
leaves scope.
Expand All @@ -22,12 +21,9 @@ is to write into the smart pointer, other times to pass as an array. To handle t
there are the methods readRef and writeRef, which are used instead of the & (ref) operator. For example

\code

Void doSomething(ID3D12Resource** resources, IndexT numResources);

// ...
ComPtr<ID3D12Resource> resources[3];

doSomething(resources[0].readRef(), SLANG_COUNT_OF(resource));
\endcode

Expand All @@ -41,45 +37,6 @@ Result res = unk->QueryInterface(resource.writeRef());
\endcode
*/

typedef SlangUUID Guid;

SLANG_FORCE_INLINE bool operator==(const Guid& aIn, const Guid& bIn)
{
// Use the largest type the honors the alignment of Guid
typedef uint32_t CmpType;
union GuidCompare
{
Guid guid;
CmpType data[sizeof(Guid) / sizeof(CmpType)];
};
// Type pun - so compiler can 'see' the pun and not break aliasing rules
const CmpType* a = reinterpret_cast<const GuidCompare&>(aIn).data;
const CmpType* b = reinterpret_cast<const GuidCompare&>(bIn).data;
// Make the guid comparison a single branch, by not using short circuit
return ((a[0] ^ b[0]) | (a[1] ^ b[1]) | (a[2] ^ b[2]) | (a[3] ^ b[3])) == 0;
}

SLANG_FORCE_INLINE bool operator!=(const Guid& a, const Guid& b)
{
return !(a == b);
}

// Allows for defining of a GUID that works in C++ and C which defines in a format similar to microsofts INTERFACE style
// MIDL_INTERFACE("00000000-0000-0000-C000-00 00 00 00 00 46")

#define SLANG_GUID_BYTE(x, index) ((uint8_t)(SLANG_UINT64(0x##x) >> (8 * index)))

#define SLANG_MAKE_GUID(data0, data1, data2, shortTail, tail) \
{ (uint32_t)(0x##data0), (uint16_t)(0x##data1), (uint16_t)(0x##data2), \
{ (uint8_t)(0x##shortTail >> 8), (uint8_t)(0x##shortTail & 0xff), \
SLANG_GUID_BYTE(tail,5), SLANG_GUID_BYTE(tail,4), SLANG_GUID_BYTE(tail,3), SLANG_GUID_BYTE(tail,2), SLANG_GUID_BYTE(tail,1), SLANG_GUID_BYTE(tail,0) \
}}

// Compatible with Microsoft IUnknown
static const Guid IID_IComUnknown = SLANG_MAKE_GUID(00000000, 0000, 0000, C000, 000000000046);

typedef ISlangUnknown IComUnknown;

// Enum to force initializing as an attach (without adding a reference)
enum InitAttach
{
Expand All @@ -92,7 +49,7 @@ class ComPtr
public:
typedef T Type;
typedef ComPtr ThisType;
typedef IComUnknown* Ptr;
typedef ISlangUnknown* Ptr;

/// Constructors
/// Default Ctor. Sets to nullptr
Expand Down Expand Up @@ -152,7 +109,7 @@ class ComPtr
protected:
/// Gets the address of the dumb pointer.
// Disabled: use writeRef and readRef to get a reference based on usage.
SLANG_FORCE_INLINE T** operator&();
SLANG_FORCE_INLINE T** operator&() = delete;

T* m_ptr;
};
Expand All @@ -168,13 +125,6 @@ void ComPtr<T>::setNull()
}
}
//----------------------------------------------------------------------------
/* template <typename T>
T** ComPtr<T>::operator&()
{
assert(m_ptr == nullptr);
return &m_ptr;
} */
//----------------------------------------------------------------------------
template <typename T>
const ComPtr<T>& ComPtr<T>::operator=(const ThisType& rhs)
{
Expand Down