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

Possibly flawed algorithm #41596

Open
gggustafson opened this issue Jun 29, 2024 · 5 comments
Open

Possibly flawed algorithm #41596

gggustafson opened this issue Jun 29, 2024 · 5 comments
Labels
doc-enhancement Improve the current content [org][type][category] dotnet-csharp/svc help wanted Good for community contributors to help [up-for-grabs] lang-reference/subsvc Pri2

Comments

@gggustafson
Copy link

gggustafson commented Jun 29, 2024

Type of issue

Other (describe below)

Description

In the "namespace IncludeTag" example, I do not believe that:

 if ((a == int.MaxValue && b > 0) || 
        (b == int.MaxValue && a > 0))

is the correct algorithm. For integers I suggest:

const int MAXINT_DIV_10 = int.MaxValue / 10;
const int MAXINT_MOD_10 = int.MaxValue % 10;

if ( !( ( a < MAXINT_DIV_10 ) ||
   ( ( a = MAXINT_DIV_10 ) && 
   ( b <= MAXINT_MOD_10 ) ) ) )
{
    throw new System.OverflowException ( );
}
 
return ( a + b );

For doubles, the algorithm is somewhat the same.

Aside: How do I get indentation to work?

Page URL

https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/xmldoc/examples

Content source URL

https://github.com/dotnet/docs/blob/main/docs/csharp/language-reference/xmldoc/examples.md

Document Version Independent Id

33d9f302-a3aa-a165-ec64-a4382feb093e

Article author

@BillWagner

Metadata

  • ID: 60a08067-b91f-3c9a-0569-30d098089990
  • Service: dotnet-csharp
  • Sub-service: lang-reference
@BillWagner
Copy link
Member

Hi @gggustafson

First the aside: If you fence code in triple-backslashes, GitHub will preserve formatting. I edited your text above as an example.

On to the main point:

The current code matches the description in the XML comments. I'd prefer keeping it the same.

@BillWagner BillWagner added the needs-more-info Needs more info from OP. Auto-closed after 2 weeks if no response. [org][resolution] label Jul 1, 2024
@dotnet-bot dotnet-bot removed the ⌚ Not Triaged Not triaged label Jul 1, 2024
@gggustafson
Copy link
Author

What happens if both parameters have the value (MAXINT / 2 + 1)? You get an exception.

All I'm concerned about are newbies thinking that the algorithm is correct (I mean it's from MS!! Isn't it?)

@github-actions github-actions bot removed the needs-more-info Needs more info from OP. Auto-closed after 2 weeks if no response. [org][resolution] label Jul 1, 2024
@dotnet-bot dotnet-bot added the ⌚ Not Triaged Not triaged label Jul 1, 2024
@adegeo
Copy link
Contributor

adegeo commented Jul 1, 2024

Hi Gus. I can see where you're coming from 😃 But I would say that the context that this code is shown in, isn't there to be an example of overflow detection nor computer science best practices, it's a simple demonstration of the documentation system. So yes, it's a flawed algorithm, but the algorithm isn't the focus of the article.

If a newbie grabbed a book on C off the shelf and there's a demonstration about doing some string operations, they may not see code guarding against every type of buffer overflow possibility, maybe just 1 or even none, depending on the context of the example. The same applies here, the context here doesn't warrant a good math library example, the math code is irrelevant except to show some simple code that can be talked about in the documentation comments above it. In this case, it shows to document the <exception> thrown by the method. None of the other methods even implement guard rails. Time, money, context, factor into how complex the example should be to demonstrate the concept.


In .NET, overflow checking is a little more complicated than just this (you may know all this, but I'm just explaining it for anyone else that runs across this issue). For example, if you're using C#, the code in the article example doesn't actually overflow, it's just an imposed overflow check. The following code runs fine and wraps the value to a negative number instead of throwing an exception:

int a = int.MaxValue / 2 + 1;
int b = int.MaxValue / 2 + 1;
Console.Write(a + b);

But if add checked in there and you'll get the exception raised automatically:

int a = int.MaxValue / 2 + 1;
int b = int.MaxValue / 2 + 1;
Console.Write(checked(a + b));

If you happen to be using VB, you'll get an exception raised by the first snippet and instead need to opt-out through a project setting.

Really the best way to do it would be to let the runtime handle it for you and force the mode you want. If you want wrapping, the .Add method would be doing return unchecked(a + b). If you wanted to throw the exception and disallow wrapping, you would do return checked(a + b).

@gggustafson
Copy link
Author

I am of the opinion that all documentation needs to be correct. I am willing to accept that as an example of documentation, this example suffices. But there needs to be a warning. If no warning then the example should be rewritten.

@BillWagner BillWagner added the doc-enhancement Improve the current content [org][type][category] label Jul 2, 2024
@dotnet-bot dotnet-bot removed the ⌚ Not Triaged Not triaged label Jul 2, 2024
@BillWagner BillWagner added help wanted Good for community contributors to help [up-for-grabs] ⌚ Not Triaged Not triaged labels Jul 2, 2024
@dotnet-bot dotnet-bot removed the ⌚ Not Triaged Not triaged label Jul 2, 2024
@BillWagner
Copy link
Member

I've added this to the backlog, and added the "help wanted" label.

I'm not against a different sample, as long as all the important doc elements are involved. However, I don't see making these updates as a priority.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc-enhancement Improve the current content [org][type][category] dotnet-csharp/svc help wanted Good for community contributors to help [up-for-grabs] lang-reference/subsvc Pri2
Projects
None yet
Development

No branches or pull requests

4 participants