bugGNU Octave - Bugs: bug #67306, use std::max() instead of ternary...

 
 

bug #67306: use std::max() instead of ternary op

Submitter:  Dmitri A. Sergatskov <dasergatskov>
Submitted:  Sat 12 Jul 2025 03:15:56 AM UTC
   
 
Category:  Coding Style and Maintenance Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Other
Status:  In Progress Assigned to:  None
Originator Name:  Open/Closed:  * Open
Release:  * dev Operating System:  * Any
Fixed Release:  11.1.0 (current default) Planned Release:  11.1.0 (current default)
* Mandatory Fields

Post a Comment

Add a New Comment Rich Markup
   

Discussion

Jump to the original submission

Tue 22 Jul 2025 07:59:39 AM UTC, comment #16: 

I know you have been using Mercurial for a long time, but some helpful instructions are on the Octave wiki at https://wiki.oc ... e.org/Mercurial.

Commit message guidelines are here https://wiki.octa ... essage_guidelines.  Getting the right balance between describing what has changed without writing an essay can sometimes be difficult.

Since you are likely to have many patches potentially in various stages of getting accepted I would use Mercurial queues (https://hgbook.re ... urial-queues.html).  That's what I do and I have  perhaps 100 patches that are in progress at any given time.

Rik <rik5>
Group administrator
Mon 21 Jul 2025 11:37:43 AM UTC, comment #15: 

Thanks. Keep it open for now.
I will try to figure out how to make cset.

Dmitri.
--

Dmitri A. Sergatskov <dasergatskov>
Mon 21 Jul 2025 09:10:02 AM UTC, comment #14: 

I combined all of the patches and checked them in under a single changeset using your name.  See https://hg.savann ... /rev/20729cd541af.

When it wasn't necessary, I used std::max without the template declaration of <octave_idx_type>.  This was mostly possible except for a few instances where there was an integer (0 or 1) which would have required casting to octave_idx_type.

Also, since you are a frequent contributor, it would be helpful if you could make full changesets (user name and commit message) rather than just patches.

Do you want to continue working on this bug report and I should leave the report open?  Otherwise, I'll close as Fixed.

Rik <rik5>
Group administrator
Tue 15 Jul 2025 06:54:54 PM UTC, comment #13: 

"CMatrix.cc" also looks straightforward.

(file das_CMatrix.diff)

Dmitri.
--


(file #57416)

Dmitri A. Sergatskov <dasergatskov>
Tue 15 Jul 2025 12:25:41 AM UTC, comment #12: 

Could not come up with anything better than:

howmany = (stride == 1) ? howmany : std::min(howmany, stride);


Still looks better than nested ternary.

The diff is attached.

(file  das_CNDArray_min.diff)

Dmitri.
--


(file #57412)

Dmitri A. Sergatskov <dasergatskov>
Tue 15 Jul 2025 12:08:21 AM UTC, comment #11: 

Nevermind, that is wrong.

Dmitri.
--

Dmitri A. Sergatskov <dasergatskov>
Mon 14 Jul 2025 11:52:06 PM UTC, comment #10: 

Although that seems to be drifting from the original subject, i spotted in a couple places in "CNDArray.cc":

howmany = (stride == 1 ? howmany : (howmany > stride ? stride : howmany));

which I think look better as

howmany = std::clamp(howmany, octave_idx_type(0), stride);


Full diff for this file incoming.

Dmitri.
--

Dmitri A. Sergatskov <dasergatskov>
Mon 14 Jul 2025 04:49:49 PM UTC, comment #9: 

Well, a patch to "CDiagMatrix.cc" looks less controversial.

(file das_cdiagmatrix_min.diff)

Dmitri.
--


(file #57411)

Dmitri A. Sergatskov <dasergatskov>
Sun 13 Jul 2025 05:17:37 PM UTC, comment #8: 

Well, you really thought I put it there because I thought it looks cool? :)

May be casting the "int" to "octave_idx_type" is better; makes it more obvious why we are doing it.

Dmitri.
--

Dmitri A. Sergatskov <dasergatskov>
Sun 13 Jul 2025 03:18:22 PM UTC, comment #7: 

Just tried various experiments with the C++ compiler and it stupidly hates the construction without template type.  How annoying.

Rik <rik5>
Group administrator
Sun 13 Jul 2025 03:12:21 PM UTC, comment #6: 

I think being explicit and calling std::max with the template type is distracting.  Given that the functional equivalent according to cplusplus.com is


The behavior of this function template (C++98) is equivalent to:

template <class T> const T& max (const T& a, const T& b) {
  return (a<b)?b:a;
}


it really is a direct replacement for the ternary operator.  The only requirement of std::max is implementation of the '<' operator.  The C++ compiler already is handling any type promotion necessary between octave_idx_type (long int) and int.  In short, I think we're good to make these changes.

Rik <rik5>
Group administrator
Sat 12 Jul 2025 06:46:31 PM UTC, comment #5: 

Since std::max() is templated, the compiler cannot deduce the type if arguments of different types like (<octave_idx_type>, <int>).
So this is hint for compiler. I guess it is possible to cast <int> into <octave_idx_type>, but I thought the way I did is better.
(But really do not have a strong opinion either way).

Dmitri.
--


Dmitri A. Sergatskov <dasergatskov>
Sat 12 Jul 2025 06:39:48 PM UTC, comment #4: 

Reviewing das_sparse_max.diff, why is it necessary to supply the template type to std::max?  For two inputs 'a' and 'b' I would normally write


std::max (a, b);


Instead, the patch has this written as


std::max<octave_idx_type> (a, b);



Rik <rik5>
Group administrator
Sat 12 Jul 2025 03:52:34 PM UTC, comment #3: 

Sure, we can leave this open.  Marking as "In Progress".

Rik <rik5>
Group administrator
Sat 12 Jul 2025 03:22:19 PM UTC, comment #2: 

May be keep it open? I have patches for other files (attached for Sparse.cc). I thought jwe said he likes a bunch of smaller patches then one big. dSparse.cc and CSparse.cc had all the usage in the same style, the Sparse.cc is different.

(file das_sparse_max.diff)

Dmitri.
--


(file #57399)

Dmitri A. Sergatskov <dasergatskov>
Sat 12 Jul 2025 03:10:53 PM UTC, comment #1: 

Seems fine.  The code for std::max effectively expands into ternary operator, but it is clearer about what is happening.  I checked the patch in under your name here https://hg.savann ... /rev/a712179b93ec.

There are probably other spots in the Octave code base where the same thing could be done.

Marking as Fixed and closing report.

Rik <rik5>
Group administrator
Sat 12 Jul 2025 03:15:56 AM UTC, original submission:  

I think it is a good idea to use `c = std::max(a,b)` construct instead of ternary op "c = (a > b ? a : b);".

Attached is the diff that make this change in dSparse.cc and CSparse.cc.

Dmitri.
--

Dmitri A. Sergatskov <dasergatskov>

 

Attached Files

Attached Files
file #57416:  das_CMatrix.diff added by dasergatskov (1.3KiB - application/octet-stream)
file #57412:  das_CNDArray_min.diff added by dasergatskov (905B - text/x-patch)
file #57411:  das_cdiagmatrix_min.diff added by dasergatskov (910B - application/octet-stream)
file #57399:  das_sparse_max.diff added by dasergatskov (2.5KiB - application/octet-stream)
file #57397:  das_max.diff added by dasergatskov (3.7KiB - application/octet-stream)

(Note: upload size limit is set to 4.0MiB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

 

Dependencies

This item does not depend on any other items.

No items depend on this one.

 

Mail Notification Carbon-Copy List

Carbon-Copy List
  • -email is unavailable- added by rik5 (Posted a comment)
  • -email is unavailable- added by dasergatskov (Submitted the item)
  •  

    Votes

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only group members can vote.

     

    History

    Follow 12 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2025-07-15 dasergatskov Attached File- Added das_CMatrix.diff, #57416
    2025-07-15 dasergatskov Attached File- Added das_CNDArray_min.diff, #57412
    2025-07-14 dasergatskov Attached File- Added das_cdiagmatrix_min.diff, #57411
    2025-07-12 rik5 StatusFixed In Progress
        Open/ClosedClosed Open
    2025-07-12 dasergatskov Attached File- Added das_sparse_max.diff, #57399
    2025-07-12 rik5 Open/ClosedOpen Closed
    2025-07-12 rik5 Planned ReleaseNone 11.1.0 (current default)
    2025-07-12 rik5 StatusNone Fixed
        Fixed ReleaseNone 11.1.0 (current default)
    2025-07-12 rik5 Item GroupNone Other
    2025-07-12 dasergatskov Attached File- Added das_max.diff, #57397

    Back to the top

    Powered by Savane 3.16-ed84.
    Corresponding source code