Skip to content

enh(NumberFormatter): Introduce backward compatible options for formatHex functions#4333

Merged
matejk merged 4 commits intodevelfrom
3667-number-formatter-hex-options
Dec 15, 2023
Merged

enh(NumberFormatter): Introduce backward compatible options for formatHex functions#4333
matejk merged 4 commits intodevelfrom
3667-number-formatter-hex-options

Conversation

@matejk
Copy link
Copy Markdown
Contributor

@matejk matejk commented Dec 11, 2023

Fixes #3667 and replaces #3657.

@matejk matejk requested a review from aleks-f December 11, 2023 13:44
@matejk matejk added this to the Release 1.13.0 milestone Dec 11, 2023
@matejk matejk self-assigned this Dec 11, 2023
@aleks-f
Copy link
Copy Markdown
Member

aleks-f commented Dec 11, 2023

@matejk @bas524 apparently, the sendFile pull did not have the VS projects regenerated, so we have CI failures not. That's why I asked for a new pull to trigger build and testrun. anyway, all affected VS projects should be regenerated with progen

     Creating library ..\lib64\PocoNet.lib and object ..\lib64\PocoNet.exp
SocketImpl.obj : error LNK2019: unresolved external symbol TransmitFile referenced in function "public: __int64 __cdecl Poco::Net::SocketImpl::sendFile(class Poco::FileInputStream &,unsigned __int64)" (?sendFile@SocketImpl@Net@Poco@@QEAA_JAEAVFileInputStream@3@_K@Z) [D:\a\poco\poco\Net\Net_vs170.vcxproj]
..\bin64\PocoNet64.dll : fatal error LNK1120: 1 unresolved externals [D:\a\poco\poco\Net\Net_vs170.vcxproj]
Error: Final attempt failed. Child_process exited with error code 1

@aleks-f aleks-f mentioned this pull request Dec 11, 2023
Merged
@matejk
Copy link
Copy Markdown
Contributor Author

matejk commented Dec 11, 2023

@matejk @bas524 apparently, the sendFile pull did not have the VS projects regenerated, so we have CI failures not. That's why I asked for a new pull to trigger build and testrun. anyway, all affected VS projects should be regenerated with progen

     Creating library ..\lib64\PocoNet.lib and object ..\lib64\PocoNet.exp
SocketImpl.obj : error LNK2019: unresolved external symbol TransmitFile referenced in function "public: __int64 __cdecl Poco::Net::SocketImpl::sendFile(class Poco::FileInputStream &,unsigned __int64)" (?sendFile@SocketImpl@Net@Poco@@QEAA_JAEAVFileInputStream@3@_K@Z) [D:\a\poco\poco\Net\Net_vs170.vcxproj]
..\bin64\PocoNet64.dll : fatal error LNK1120: 1 unresolved externals [D:\a\poco\poco\Net\Net_vs170.vcxproj]
Error: Final attempt failed. Child_process exited with error code 1

I'll take care first thing in the morning since I messed up.

@matejk matejk force-pushed the 3667-number-formatter-hex-options branch from 820b82c to 4a6366e Compare December 12, 2023 21:14
@matejk
Copy link
Copy Markdown
Contributor Author

matejk commented Dec 14, 2023

@aleks-f, are there any comments from your side on this PR or can I merge to devel?

@matejk matejk requested a review from teksturi December 14, 2023 18:05
@matejk matejk force-pushed the 3667-number-formatter-hex-options branch 2 times, most recently from 35944fe to b288f6e Compare December 14, 2023 18:06
teksturi
teksturi reviewed Dec 14, 2023
View reviewed changes
teksturi
teksturi reviewed Dec 14, 2023
View reviewed changes
teksturi
teksturi reviewed Dec 14, 2023
View reviewed changes
teksturi
teksturi reviewed Dec 14, 2023
View reviewed changes
teksturi
teksturi reviewed Dec 14, 2023
View reviewed changes
teksturi
teksturi reviewed Dec 14, 2023
View reviewed changes
teksturi
teksturi reviewed Dec 14, 2023
View reviewed changes
Foundation/include/Poco/NumberFormatter.h Outdated
/// The value is treated as unsigned.

[[deprecated("use formatHex with options instead")]]
static std::string formatHex(int value, int width, bool prefix);
Copy link
Copy Markdown
Contributor

@teksturi teksturi Dec 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So why do you remove default prefix false? That change will break things if someone is using these functions. They have been in 11 years already. Surely someone is relaying them. And if we remove those why to depracate these functions even in the first place if user still get broken build.

Copy link
Copy Markdown
Member

@aleks-f aleks-f Dec 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree - deprecated functions' reason for existence is precisely not to break code and give people time to adjust

Copy link
Copy Markdown
Contributor Author

@matejk matejk Dec 15, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed the default prefix because there is ambiguity and compile fails.

[[deprecated("use formatHex with options instead")]]
static std::string formatHex(int value, int width, bool prefix = false);
static std::string formatHex(int value, int width, Options options = Options::OPT_NONE);

Default was no prefix which is also the default when using options. The behaviour for the user if it did not use the prefix does not change. Deprecation warning is issued in cases when bool prefix argument was explicitly used. All situations including defaults are tested with unit tests.

/Users/matejk/git/github/poco/Foundation/src/Logger.cpp:252:18: error: call to 'formatHex' is ambiguous
                message.append(NumberFormatter::formatHex(addr, 4));
                               ^~~~~~~~~~~~~~~~~~~~~~~~~~
/Users/matejk/git/github/poco/Foundation/include/Poco/NumberFormatter.h:704:37: note: candidate function
inline std::string NumberFormatter::formatHex(int value, int width, bool prefix)
                                    ^
/Users/matejk/git/github/poco/Foundation/include/Poco/NumberFormatter.h:710:37: note: candidate function
inline std::string NumberFormatter::formatHex(int value, int width, Options options)
                                    ^

Copy link
Copy Markdown
Contributor

@teksturi teksturi Dec 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aa true. Nice

aleks-f
aleks-f requested changes Dec 15, 2023
View reviewed changes
Foundation/include/Poco/NumberFormatter.h Outdated
/// The value is treated as unsigned.

[[deprecated("use formatHex with options instead")]]
static std::string formatHex(int value, int width, bool prefix);
Copy link
Copy Markdown
Member

@aleks-f aleks-f Dec 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree - deprecated functions' reason for existence is precisely not to break code and give people time to adjust

@matejk matejk force-pushed the 3667-number-formatter-hex-options branch from ccdc7ec to 208ae0f Compare December 15, 2023 12:50
@matejk
Copy link
Copy Markdown
Contributor Author

matejk commented Dec 15, 2023

@teksturi and @aleks-f, thank you very much for your reviews.

I pushed corrected code. Can you please take a look if it is OK to merge?

teksturi
teksturi approved these changes Dec 15, 2023
View reviewed changes
@matejk matejk force-pushed the 3667-number-formatter-hex-options branch from 208ae0f to a127656 Compare December 15, 2023 17:29
@matejk matejk merged commit 111fe90 into devel Dec 15, 2023
@matejk matejk deleted the 3667-number-formatter-hex-options branch December 15, 2023 17:31
@teksturi teksturi mentioned this pull request Dec 17, 2023
Open
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@aleks-f aleks-f aleks-f requested changes

@teksturi teksturi teksturi approved these changes

Assignees

@matejk matejk

Labels

None yet

Projects

None yet

Milestone

Release 1.13.0

Development

Successfully merging this pull request may close these issues.

NumberFormatter: add Options enum for controlling prefix and lowercase

3 participants

@matejk @aleks-f @teksturi