Skip to content

Conversation

@andraantariksa
Copy link
Member

No description provided.


"include/tempeh/gpu/vk/device.hpp"
"include/tempeh/gpu/vk/vk.hpp"
"include/tempeh/gpu/vk/volk.h")
Copy link
Member

Choose a reason for hiding this comment

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

volk.h should not be here

Copy link
Member

@native-m native-m left a comment

Choose a reason for hiding this comment

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

I revert this changes and will never merge this to the master. If you are using the master branch as your base for the implementation, it's incomplete. Please use vulkan-impl branch as the base of WebGPU backend implementation, it contains complete (almost) interfaces.

#include "vk.hpp"

namespace Tempeh::GPU
namespace Tempeh::GPU::Vk
Copy link
Member

@native-m native-m Feb 9, 2022

Choose a reason for hiding this comment

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

Do not add too many namespaces.
Seriously, you can always put the name of the backend at the end of the derived class name.
Deep namespace is unnecessary in this case.
These classes are not going to be visible to the end-user anyway, so adding the namespace is completely useless.

Copy link
Member Author

Choose a reason for hiding this comment

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

Namespace good. If its too long, you can just use alias

Copy link
Member

@native-m native-m Feb 9, 2022

Choose a reason for hiding this comment

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

Not good, alias can add redundant lines. It's just not worth it.
Why add deep namespaces when can just DeviceVK. Tbh, it's not worth it in this case.

@native-m native-m closed this Feb 9, 2022
using Box = std::unique_ptr<T>;

template<typename T>
FUNCTION_ALIAS(make_rc, std::make_shared<T>);
Copy link
Member

@native-m native-m Feb 9, 2022

Choose a reason for hiding this comment

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

make_shared & make_unique requires you to provide the constructor parameters, this is probably wont work for classes that don't have a default constructor.

@native-m native-m reopened this Feb 9, 2022
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.

3 participants