Skip to content

Commit

Permalink
Fix MSVC compilation of b60cb19
Browse files Browse the repository at this point in the history
  • Loading branch information
@lhog
lhog committed Aug 1, 2023
1 parent b60cb19 commit c5f8474
Showing 1 changed file with 4 additions and 3 deletions.
7 changes: 4 additions & 3 deletions rts/Rendering/GL/glHelpers.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,10 @@
template<class GLType>
inline void glGetAny(GLenum paramName, GLType* data, const int expectedValuesN = -1)
{
GLint ints[expectedValuesN];
GLint ints[1024];
assert(expectedValuesN > 0 && expectedValuesN < 1024);
glGetIntegerv(paramName, ints);
std::move(ints, ints+expectedValuesN, data);
std::copy(ints, ints+expectedValuesN, data);
}
template<>
inline void glGetAny<GLint>(GLenum paramName, GLint* data, const int)
Expand Down Expand Up @@ -84,7 +85,7 @@ inline void glSetAny(AttribValuesTupleType newValues)
{
if constexpr(DedicatedGLFuncPtrPtr)
{
std::apply(*DedicatedGLFuncPtrPtr, newValues);
std::apply(DedicatedGLFuncPtrPtr, newValues);
}
else // glEnable/glDisable(attribute)
{
Expand Down

7 comments on commit c5f8474

@Krogoth100
Copy link
Contributor

@Krogoth100 Krogoth100 commented on c5f8474 Aug 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

No, these are incorrect fixes. It should not work like that, especially that pointer change.
Can you show me exact errors on msvc.

@Krogoth100
Copy link
Contributor

Choose a reason for hiding this comment

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

Could it be somehow related that it is ".h" and msvc doesn't understand that its a C++ file.

@Krogoth100
Copy link
Contributor

@Krogoth100 Krogoth100 commented on c5f8474 Aug 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Try .hpp, to be sure.
I cannot reproduce this myself because I don't have msvc setup.

@Krogoth100
Copy link
Contributor

Choose a reason for hiding this comment

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

Make sure you are using correct GCC version. std::move error seems like a case of old GCC, that doesn't have algorithmic move and mistakes it for utility std::move.

@lhog
Copy link
Collaborator Author

@lhog lhog commented on c5f8474 Aug 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

wdym incorrect?

  • GLint ints[expectedValuesN]; doesn't form a constant expression, MSVC dislikes that, GCC is more tolerant, but I think in that case MSVC is making a right call.
  • std::apply() change should be fine, even if you pass function type by value it's implicitly converted into a pointer behind the scenes, no need to cast it back to a value type.
  • With std::move I'm just not used to that form of std::move, here it's not different from std::copy, mostly a cosmetic change as far as I can tell.

@sprunk
Copy link
Collaborator

@sprunk sprunk commented on c5f8474 Aug 1, 2023

Choose a reason for hiding this comment

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

  • GLint ints[expectedValuesN]; is a VLA (variable length array) which is a thing in plain C but not C++. GCC offers it by default as an extension. expectedValuesN is declared as const though, perhaps what was wanted was a compile-time constant? Then using it as the array size would be correct:
template <size_t expectedValuesN, class GLType>
inline void glGetAny(GLenum paramName, GLType* data) {
  GLint ints[expectedValuesN]; // OK, compile-time template parameter, not VLA

  • For std::apply the parameter is called DedicatedGLFuncPtrPtr (note double "Ptr"), will a double-depth pointer decay to the function correctly? Also will this still work if you pass something like a lambda or a functor object?

  • For std::move in this case it seems to be equivalent since GLint is just some flavor of int, though I like the use of std::move to denote the intent (we no longer care about the original table and will not using it anymore).

@Krogoth100
Copy link
Contributor

@Krogoth100 Krogoth100 commented on c5f8474 Aug 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

  • GLint ints[expectedValuesN]; is a VLA (variable length array) which is a thing in plain C but not C++. GCC offers it by default as an extension. expectedValuesN is declared as const though, perhaps what was wanted was a compile-time constant? Then using it as the array size would be correct:
template <size_t expectedValuesN, class GLType>
inline void glGetAny(GLenum paramName, GLType* data) {
  GLint ints[expectedValuesN]; // OK, compile-time template parameter, not VLA
  • For std::apply the parameter is called DedicatedGLFuncPtrPtr (note double "Ptr"), will a double-depth pointer decay to the function correctly? Also will this still work if you pass something like a lambda or a functor object?
  • For std::move in this case it seems to be equivalent since GLint is just some flavor of int, though I like the use of std::move to denote the intent (we no longer care about the original table and will not using it anymore).

You are right about VLA. I changed template parameter into a function argument and didn't think it through.
I shall take another look at this.

std::move - this is purely for intent, yes. std::copy is the same.

I 99% sure apply will not work in this case.

Edit:
Apparently it somehow does

Please sign in to comment.