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

MINOR: [R] Remove whitespace after operator"" #45756

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

MichaelChirico
Copy link
Contributor

Rationale for this change

The space after "" is deprecated since it creates ambiguity with reserved _-initial names per:

https://en.cppreference.com/w/cpp/language/user_literal

What changes are included in this PR?

Whitespace change:

-operator"" _nm
+operator""_nm

Are these changes tested?

No

Are there any user-facing changes?

No

Copy link

Thanks for opening a pull request!

If this is not a minor PR. Could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose

Opening GitHub issues ahead of time contributes to the Openness of the Apache Arrow project.

Then could you also rename the pull request title in the following format?

GH-${GITHUB_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}

or

MINOR: [${COMPONENT}] ${SUMMARY}

See also:

@MichaelChirico
Copy link
Contributor Author

Looks like this is a follow-up to #45606 cc @jonkeane

@MichaelChirico MichaelChirico changed the title [R] Remove whitespace after operator"" MINOR: [R] Remove whitespace after operator"" Mar 12, 2025
@jonkeane
Copy link
Member

This looks fine, but I'm surprised that it's not giving the warning when compiling on clang20 (https://github.com/ursacomputing/crossbow/actions/runs/13801015462/job/38603309193 for last night). I'll approve + merge this, but curious if you know why this isn't being warned?

@MichaelChirico
Copy link
Contributor Author

Beats me, I only did a regex GH search to find this.

Maybe it only complains about declarations, not using form? Did the other one in r/src/filesystem.cpp get caught by the warning being thrown?

@MichaelChirico
Copy link
Contributor Author

MichaelChirico commented Mar 12, 2025

NVM, I do see it in the logs, it's just not erroring (?):

  #21 148.7 ../inst/include/date/date.h:966:34: warning: identifier '_d' preceded by whitespace in a literal operator declaration is deprecated [-Wdeprecated-literal-operator]
  #21 148.7   966 | CONSTCD11 date::day  operator "" _d(unsigned long long d) NOEXCEPT;
  #21 148.7       |                      ~~~~~~~~~~~~^~
  #21 148.7       |                      operator""_d
  #21 148.7 ../inst/include/date/date.h:967:34: warning: identifier '_y' preceded by whitespace in a literal operator declaration is deprecated [-Wdeprecated-literal-operator]
  #21 148.7   967 | CONSTCD11 date::year operator "" _y(unsigned long long y) NOEXCEPT;
  #21 148.7       |                      ~~~~~~~~~~~~^~
  #21 148.7       |                      operator""_y
  #21 148.7 ../inst/include/date/date.h:1975:13: warning: identifier '_d' preceded by whitespace in a literal operator declaration is deprecated [-Wdeprecated-literal-operator]
  #21 148.7  1975 | operator "" _d(unsigned long long d) NOEXCEPT
  #21 148.7       | ~~~~~~~~~~~~^~
  #21 148.7       | operator""_d
  #21 148.7 ../inst/include/date/date.h:1983:13: warning: identifier '_y' preceded by whitespace in a literal operator declaration is deprecated [-Wdeprecated-literal-operator]
  #21 148.7  1983 | operator "" _y(unsigned long long y) NOEXCEPT
  #21 148.7       | ~~~~~~~~~~~~^~
  #21 148.7       | operator""_y

(searched the logs you shared for deprecated-literal-operator)

(NVM again, I don't see any filesystem.cpp: annotations, I think we're back to "only warns on declarations, not using")

@jonkeane
Copy link
Member

(NVM again, I don't see any filesystem.cpp: annotations, I think we're back to "only warns on declarations, not using")

nods yeah, and it doesn't bubble up in https://www.r-project.org/nosvn/R.check/r-devel-linux-x86_64-fedora-clang/arrow-00check.html either. Like I said, I think it's not problematic to merge this regardless, but odd that it's not flagging

@MichaelChirico
Copy link
Contributor Author

I asked the LLMs and they are just BSing (as anticipated), though Claude happens on the right answer at least

https://claude.ai/share/138e5823-cdc8-471d-bd09-c43759396027

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants