Your updates look good to me. We can go ahead and merge these changes. Thank you very much for your help!
Best regards, Ahmet ________________________________ From: H.J. Lu <[email protected]> Sent: Monday, October 6, 2025 6:12 PM To: Akkas, Ahmet <[email protected]> Cc: Cornea, Marius <[email protected]>; Hongtao Liu <[email protected]>; GCC Patches <[email protected]>; Liu, Hongtao <[email protected]>; Anderson, Cristina S <[email protected]> Subject: Re: PING: [PATCH] libbid: Set rounding mode to round-to-nearest for _Decimal128 arithmetic On Tue, Oct 7, 2025 at 8:15 AM Akkas, Ahmet <[email protected]> wrote: > > Based on my initial review, I would like to mention two points. > > I understand the reason why do { ... } while (0) structure is used for macro > definition - to have a single block. On the other hand, we have the following > code where I believe it is better to move DFP_GET_ROUNDMODE into do-while > block. The reason is that if the DFP_INIT_ROUNDMODE is used in if statement > without parenthesis then it can trigger a “dangling else” issue I think. > > > /* Initialize the rounding mode to round-to-nearest if needed. */ > #define DFP_INIT_ROUNDMODE \ > DFP_GET_ROUNDMODE; \ We can't move DFP_GET_ROUNDMODE inside of the do-while block since DFP_GET_ROUNDMODE declares unsigned int _frnd_orig; which will be used by DFP_SET_ROUNDMODE (_frnd_orig); in DFP_RESTORE_ROUNDMODE later. > do \ > { \ > if (_frnd_orig != FP_RND_NEAREST) \ > DFP_SET_ROUNDMODE (FP_RND_NEAREST); \ > } \ > while (0) > > The similar update can also be applied to the variable declaration. For > example, in the following code I think we can move unsigned int _frnd_orig > into the do-while block. > > /* Get the rounding mode. */ > #define DFP_GET_ROUNDMODE \ > unsigned int _frnd_orig; \ > do \ > { \ > __asm__ __volatile__ ("%vstmxcsr\t%0" : "=m" (_frnd_orig)); \ > _frnd_orig &= FP_RND_MASK; \ > } \ > while (0) > > > The second point is about having semicolon at the end of while(0). In one > case, we have semicolon at the end of while(0) but we do not have it for the > other instances. It might be a good idea to make them same/consistent - > assuming that would not cause any build issue. I added ';' in the v2 patch. OK for master if there are no regressions? -- H.J.
