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

Closes #2593: Add compat version of c_void_ptr for new c_ptr(void) #2594

Merged
merged 2 commits into from
Jul 20, 2023
Merged

Closes #2593: Add compat version of c_void_ptr for new c_ptr(void) #2594

merged 2 commits into from
Jul 20, 2023

Conversation

riftEmber
Copy link
Contributor

@riftEmber riftEmber commented Jul 18, 2023

Chapel recently deprecated c_void_ptr in favor of
c_ptr(void), which will be in the 1.32 release
(PR chapel-lang/chapel#22637).
This also included compiler changes to make
c_ptr(void) possible, which would not previously
compile. To avoid use of the now-deprecated
c_void_ptr in versions newer than 1.31, introduce
a new c_ptr_void symbol in ArkoudaCTypesCompat
which is c_void_ptr for Chapel <=1.31, and
c_ptr(void) for Chapel >1.31.

Note this is based off of
#2589 which adds
the new compat dir for 1.32, to avoid conflicts.
This PR should be merged after that one.

Closes #2593

Copy link
Member

@stress-tess stress-tess left a comment

Choose a reason for hiding this comment

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

Looks good to me! As @riftEmber said, we should prob try and merge #2589 since this is built on that

Copy link
Contributor

@Ethan-DeBandi99 Ethan-DeBandi99 left a comment

Choose a reason for hiding this comment

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

I am a bit worried about this moving forward. Typically, the compat for the current version would have nothing in it and then the ones for any older versions would be configured so that the new call returns the old thing. So I would be expecting the gt131 compat to be empty and e-130 and eq-131 to have c_ptr(void) defined to return the old c_void_ptr. This is introducing a lot of updates that will need to be made again in the future because c_ptr_void is not what is used in any Chapel version. I am also a bit concerned with the naming that it will not be an obvious difference to people. Is there a reason we have to do it this way?

@riftEmber
Copy link
Contributor Author

@Ethan-DeBandi99 Unfortunately I believe there is no way in Chapel to define c_ptr(void) to mean c_void_ptr, as c_ptr(void) is not an identifier but a type constructor call; type c_ptr(void) = c_void_ptr is a syntax error. Given this, as well as the fact that using c_ptr(void) only compiles in Chapel >1.31, I don't think there's a way around using a separate symbol to refer to c_ptr(void) and c_void_ptr depending on version.

There might be a better name than c_ptr_void, but it's the clearest one I was able to come up with so far. I did try using type c_void_ptr = c_ptr(void) in gt-131 so Arkouda could just continue using the symbol name c_void_ptr, but this doesn't work as it's a redefinition of the deprecated but still existing c_void_ptr in CTypes.

I think the future work created by this is:

  • for each newly-supported Chapel version until 1.31 is dropped, there needs to be a CTypes compat module containing type c_ptr_void = c_ptr(void)
  • once Arkouda drops support for 1.31, c_ptr_void compat stuff can go away and c_ptr(void) can be used in Arkouda code

I recognize this change in Chapel, which I was responsible for, is relatively abrupt and a nuisance to work around, and I apologize for that. The reason for it is time constraints in needing to complete our potentially-disruptive changes to the CTypes module by release 1.32.

@stress-tess stress-tess requested a review from bmcdonald3 July 19, 2023 17:29
@riftEmber
Copy link
Contributor Author

Hold on, I may have been wrong about not being able to re-declare type c_void_ptr = c_ptr(void). I experimented again in a test program and it seems to let me; I'll check if maybe I just did something else wrong when trying it for Arkouda, since if possible that's probably a better option.

@stress-tess
Copy link
Member

Hold on, I may have been wrong about not being able to re-declare type c_void_ptr = c_ptr(void). I experimented again in a test program and it seems to let me; I'll check if maybe I just did something else wrong when trying it for Arkouda, since if possible that's probably a better option.

Yah if that's possible, that would be awesome!

@riftEmber
Copy link
Contributor Author

riftEmber commented Jul 19, 2023

Ok, it is not possible in the way I was hoping. My confusion came from the fact that for a private use/use, symbols are introduced in an implicit outer "shadow" scope, and names can therefore be shadowed. Whereas for a public use as is necessary here there is no shadow scope and shadowed names are conflicts. So for this case we really can't do a type c_void_ptr = c_ptr(void) alias. Here is the relevant Chapel spec section.

I did come up with another alternative, which makes the gt-131 compat module a little messy/hacky but allows use of c_void_ptr uniformly in Arkouda code until 1.31 support is dropped:

// compat/gt-131/ArkoudaCTypesCompat.chpl
module ArkoudaCTypesCompat {
  // Use an only clause including everything except the deprecated c_void_ptr,
  // so we can define it as an alias for its replacement c_ptr(void).
  // Using an except clause would not work here as the reference to the
  // deprecated name c_void_ptr would warn.
  public use CTypes only c_float, c_double, cFileTypeHasPointer, c_FILE, c_ptr,
         c_ptrConst, c_array, c_ptrTo, c_ptrToConst, cPtrToLogicalValue,
         c_addrOf, c_addrOfConst, c_sizeof, c_offsetof, allocate, deallocate;
  // Need to manually use ChapelSysCTypes here, as it is `public use`d in CTypes
  // but doesn't get picked up by the above use with only clause.
  public use ChapelSysCTypes;

  // Redefine c_void_ptr to a usable form, without deprecation warning.
  type c_void_ptr = c_ptr(void);
}

This would eliminate future work, except for replacing c_void_ptr with c_ptr(void) and removing this compat code at your convenience after 1.31 support is dropped. Is this solution preferable to the c_ptr_void option?

@stress-tess
Copy link
Member

stress-tess commented Jul 19, 2023

This would eliminate future work, except for replacing c_void_ptr with c_ptr(void) and removing this compat code at your convenience after 1.31 support is dropped. Is this solution preferable to the c_ptr_void option?

I think either path forward seems reasonable, so I'll defer to @Ethan-DeBandi99 on this one

@Ethan-DeBandi99
Copy link
Contributor

@riftEmber - Thank you for providing so much information of this. This is extremely helpful. My personal preference would be to use the existing c_void_ptr and update that in the future. This way we don't have to get used to 2 new formats, we will only need to update to c_ptr(void) when we no longer support older versions.

Again, thank you for looking into this more.

@riftEmber
Copy link
Contributor Author

riftEmber commented Jul 20, 2023

No problem! I've implemented the c_void_ptr route and it's compiling for me locally without the deprecation warnings. Let me know if there's any more revision that needs to be done on this.

Copy link
Member

@stress-tess stress-tess left a comment

Choose a reason for hiding this comment

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

Looks good! Thanks!

@stress-tess stress-tess enabled auto-merge July 20, 2023 17:29
@stress-tess stress-tess added this pull request to the merge queue Jul 20, 2023
Merged via the queue into Bears-R-Us:master with commit 418e75a Jul 20, 2023
@riftEmber riftEmber deleted the c_void_ptr-132-compat branch July 10, 2024 19:53
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.

Add compat version of c_void_ptr for new c_ptr(void)
3 participants