4 ms·

The PRs they link mostly seem like noise? “Remove the d prefix from this number because the C++ standard doesn’t require it”. Yeah great.

deleted 9mo ago
[deleted]
jstanley 9mo ago
That's a pretty unfair characterisation of the commit in question: https://github.com/loongson/jdk/pull/125/commits/ee300a6ce7348f77c2642f3a6b5b0c46d6117862 https://github.com/loongson/jdk/pull/125/commits/ee300a6ce73... By my reading, it's not merely that the standard doesn't require the "d" suffix, it's that the standard doesn't allow the "d" suffix, and the code won't compile on anything but gcc.
- freedomben 9mo ago
  Agreed, although things I immediately think of are: 1. Is "anything but gcc" actually supported by the project? Do they have a goal of supporting other compilers or possibly an explicit decision not to support other compilers? 2. If they do support other compilers, how did the "d" suffix make it in the first place? That's something I would expect the dev or CI to catch pretty quickly. 3. Does gcc behave any differently with the "d" suffix not there? (I would think a core dev would know that off the top of their head, so it's possible they looked at it and decided it wasn't worth it. One would hope they'd comment on the PR though if they did that). If it does, this could introduce a really hard-to-track-down bug. I'm not defending Oracle here (in fact I hate Oracle and think they are a scourge on humanity) but trying to approach this with an objective look.
  - mort96 9mo ago
    Is the project clearly documented as being written in GNU C++ rather than standard C++? If not, anything that's accidentally invalid C++ is fair game for bug fixes, is it not?
  - dundarious 9mo ago
    Given they have one to fix usage of llvm-config, I assume clang is also supported or being worked on.
    - stuaxo 9mo ago
      That sort of patch is clearly fixing something that blocked him, and probably blocked many others who didn't get as far as trying to fix it. A project should take on useful small patches, thats how you onboard contributors.
      - gf000 9mo ago
        That again assumes a project is looking to onboard contributors. I absolutely get that it was an unfortunate interaction from the email writer's perspective, and it's really unfortunate. But there are a lot of concerns/bureaucracy, etc in case of large projects like this. It may just never got to the person responsible, because it is a cross-cutting concern (so no clear way to assign it to someone) with a low priority.
        dundarious 9mo ago
        They keep stringing him along in the process to onboard him as a contributor. The issue is the split personality in wanting but not acting on onboarding, with no meaningful communication. Your last paragraph about bureaucracy is exactly the complaint of the post. I don't see it as a defense. We can all throw our hands up and say "shit happens", and we can all agree it invariably does happen sometimes, but it's not a defense, per se.
- dwroberts 9mo ago
  If all of these things are about making it build under clang though they need to better explain it or maybe group these changes together though. My initial comment was maybe unfair but I can completely sympathise with the maintainers etc. that separately these PRs look like random small edits (e.g. from a linter) with no specific goal
  - imcritic 9mo ago
    Shouldn't small trivial changes be easier to review (and thus maybe even have higher prio)?
    - gf000 9mo ago
      If there is a single maintainer of the project, sure. If it's such a massively huge project like OpenJDK, then not really. You might also check how non-trivial it is to get a change into the Linux kernel.
thethirdone 9mo ago
The d suffix makes it not compile under clang. The PRs seem like mostly small changes that are clear improvements.
ablob 9mo ago
The correct quote is: "Remove invalid 'd' suffix for double literals".
perryprog 9mo ago
Even if the changes aren't "meaningful" (which it seems like they are), they still have an impact in how it makes the contributor more comfortable with working on the project. No new contributor is going to start with making massive patches without starting out with some smaller things to get a feel for working with the project.
- Twirrim 9mo ago
  Agreed, these seem like ideal patches to me for a first contribution. Solves a specific problem, doesn't require a lot of effort on maintainers side to review, and should give them a straightforward path to familiarise themselves with the process.

deleted 9mo ago

[deleted]

jstanley 9mo ago

That's a pretty unfair characterisation of the commit in question: https://github.com/loongson/jdk/pull/125/commits/ee300a6ce7348f77c2642f3a6b5b0c46d6117862 https://github.com/loongson/jdk/pull/125/commits/ee300a6ce73... By my reading, it's not merely that the standard doesn't require the "d" suffix, it's that the standard doesn't allow the "d" suffix, and the code won't compile on anything but gcc.

freedomben 9mo ago
Agreed, although things I immediately think of are: 1. Is "anything but gcc" actually supported by the project? Do they have a goal of supporting other compilers or possibly an explicit decision not to support other compilers? 2. If they do support other compilers, how did the "d" suffix make it in the first place? That's something I would expect the dev or CI to catch pretty quickly. 3. Does gcc behave any differently with the "d" suffix not there? (I would think a core dev would know that off the top of their head, so it's possible they looked at it and decided it wasn't worth it. One would hope they'd comment on the PR though if they did that). If it does, this could introduce a really hard-to-track-down bug. I'm not defending Oracle here (in fact I hate Oracle and think they are a scourge on humanity) but trying to approach this with an objective look.
- mort96 9mo ago
  Is the project clearly documented as being written in GNU C++ rather than standard C++? If not, anything that's accidentally invalid C++ is fair game for bug fixes, is it not?
- dundarious 9mo ago
  Given they have one to fix usage of llvm-config, I assume clang is also supported or being worked on.
  - stuaxo 9mo ago
    That sort of patch is clearly fixing something that blocked him, and probably blocked many others who didn't get as far as trying to fix it. A project should take on useful small patches, thats how you onboard contributors.
    - gf000 9mo ago
      That again assumes a project is looking to onboard contributors. I absolutely get that it was an unfortunate interaction from the email writer's perspective, and it's really unfortunate. But there are a lot of concerns/bureaucracy, etc in case of large projects like this. It may just never got to the person responsible, because it is a cross-cutting concern (so no clear way to assign it to someone) with a low priority.
      - dundarious 9mo ago
        They keep stringing him along in the process to onboard him as a contributor. The issue is the split personality in wanting but not acting on onboarding, with no meaningful communication. Your last paragraph about bureaucracy is exactly the complaint of the post. I don't see it as a defense. We can all throw our hands up and say "shit happens", and we can all agree it invariably does happen sometimes, but it's not a defense, per se.
dwroberts 9mo ago
If all of these things are about making it build under clang though they need to better explain it or maybe group these changes together though. My initial comment was maybe unfair but I can completely sympathise with the maintainers etc. that separately these PRs look like random small edits (e.g. from a linter) with no specific goal
- imcritic 9mo ago
  Shouldn't small trivial changes be easier to review (and thus maybe even have higher prio)?
  - gf000 9mo ago
    If there is a single maintainer of the project, sure. If it's such a massively huge project like OpenJDK, then not really. You might also check how non-trivial it is to get a change into the Linux kernel.

thethirdone 9mo ago

The d suffix makes it not compile under clang. The PRs seem like mostly small changes that are clear improvements.

ablob 9mo ago

The correct quote is: "Remove invalid 'd' suffix for double literals".

perryprog 9mo ago

Even if the changes aren't "meaningful" (which it seems like they are), they still have an impact in how it makes the contributor more comfortable with working on the project. No new contributor is going to start with making massive patches without starting out with some smaller things to get a feel for working with the project.

Twirrim 9mo ago
Agreed, these seem like ideal patches to me for a first contribution. Solves a specific problem, doesn't require a lot of effort on maintainers side to review, and should give them a straightforward path to familiarise themselves with the process.