On Thu, 20 Feb 2025 19:38:29 GMT, Harshitha Onkar <hon...@openjdk.org> wrote:
>>> Can't this field be final and be set in constructor of the class? >>> > Indeed, it _can be_, and I prefer this design. This way, the `isBuiltIn` >>> > field can't be changed after the object is constructed. I think it's a >>> > cleaner design. >> >> A cleaner diff between your latest commit and my commit on top: >> https://github.com/openjdk/jdk/compare/8da82c10b5be3b92655abef95fd5be0c8b6eaa00..6729625f39c0dc47cd2588906b1793611996ca10 >> >> This link shouldn't become invalid after either of us removes the >> corresponding branches from our forks. > >> Indeed, it can be, and I prefer this design. This way, the isBuiltIn field >> can't be changed after the object is constructed. I think it's a cleaner >> design. > > I agree, the approach you mentioned does have merits. After evaluating both > approaches setting in static block was preferred over the constructor for the > following reason. > Setting it via constructor meant that we could mark the profile as Built-In > only when it was constructed using ProfileDeferralInfo and modifying all the > constructors - ICC_Profile, ICC_ProfileRGB, ICC_ProfileGray whereas setting > it in static block meant minimal changes to existing code. I don't get the reason why it's bad to modify the constructors. At this time, all the built-in profiles are created with `ProfileDeferralInfo`. If `ProfileDeferralInfo` is used to create only built-in profiles, then modifying `ICC_Profile(ProfileDeferralInfo pdi)` is enough: ICC_Profile(ProfileDeferralInfo pdi) { deferralInfo = pdi; isBuiltIn = true; } And this is a *minimal* change. When the `isBuiltIn` is final, it's much easier to find where the value gets set. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/23606#discussion_r1964330041
