Basic support for Unicode file names and some bugfixes by aafemt · Pull Request #15 · FirebirdSQL/firebird

aafemt · 2016-04-16T12:12:16Z

Unicode file names and pathes work at API level. Little glitches can be fixed after Pavel Zotov create test suite.
Much more must be done for unicode support in utilities command lines.

AlexPeshkoff · 2016-06-16T16:55:14Z

@@ -411,10 +411,10 @@ namespace Firebird
 		}
 		void insert(size_type p0, const_pointer s, const size_type n)
 		{


IMO this fix should be done as separate pull request and backported to 2.5 and 3.0.

No, this is regression of this branch, problem does not exist in other branches.

On 06/16/2016 07:58 PM, Dimitry Sibiryakov wrote:

@@ -411,10 +411,10 @@ namespace Firebird
}
void insert(size_type p0, const_pointer s, const size_type n)
{
No, this is regression of this branch, problem does not exist in other branches.

OK, in that case no problems.

egorpugin · 2016-07-16T22:49:48Z

Why just not use boost::filesystem and remove tons of code?
Especially when boost::filesystem was accepted into C++17.

hvlad · 2016-08-12T09:44:51Z

What issue the last commit (2f89d89) solved ? Just for fun ?

aafemt · 2016-08-12T09:47:17Z

It got rid of magic numbers before Adriano requested that.

egorpugin · 2016-08-12T09:55:21Z

Size of dllExt string is known at compile time, so it's better to use sizeof() operator.

aafemt · 2016-08-12T09:57:20Z

So, strlen also will be calculated at compile time. But in contrast with sizeof it will return right value, not size of pointer.

egorpugin · 2016-08-12T10:07:03Z

sizeof for static const char dllExt[] = ".dll";, not for const char *.
Why strlen() is compile time?

aafemt · 2016-08-12T10:08:30Z

Because compilers are clever nowadays.

egorpugin · 2016-08-12T10:15:30Z

It will probably require modern constexpr keyword.
By the way on line 195 of 2f89d89 you can use also dllExt and not ".dll".

String refactoring as requested by Alex.

AlexPeshkoff · 2016-10-18T08:01:08Z

 								a3.printf("GroupName=%s\n", U.PLG$GROUP_NAME);

-							attr = a1 + a2 + a3;
+							attr = a1;


This change has nothing to do with unicode support

Specially strange taken together with introduced by you assignment operator using rvalue reference. With this c++11 feature I see absolutely no sense in this change.

AlexPeshkoff · 2016-10-18T08:10:25Z

 										SpecificAttributesMap* map);

-	static string convertAsciiToUtf16(const string& ascii);
+	static string convertAsciiToUtf16(const char* ascii);


What's a reason in using here plain char pointer instead class reference?

aafemt · 2016-10-18T09:14:40Z

This change has nothing to do with unicode support

Yes, this is result of string refactoring which you demanded.

aafemt · 2016-10-18T09:17:19Z

What's a reason in using here plain char pointer instead class reference?

This routine used to be called using string literal. Creating of temporary class is
unnecessary overhead. In rare cases when it is called with data contained in string, call
of c_str() has no overhead.

aafemt · 2016-10-18T11:00:17Z

Specially strange taken together with introduced by you assignment operator using rvalue
reference.

This operator has no advantage on short strings which fit into internal buffer. That's why
control of assignments is still important for performance.

AlexPeshkoff · 2016-10-18T11:01:17Z

+		}

-		AbstractString& append(const AbstractString& str)
+		void append(const AbstractString& str)


Rollback this massive changes from 'AbstractString&' to 'void' return type. Not related with unicode support (and violate syntax of std::basic_string).

These routines are private. They cannot be called from outside code and have nothing to do with public interface of std::basic_string except name. Their public envelopes in derived classes return reference to instance of proper class.

AlexPeshkoff · 2016-10-18T11:02:53Z

On 10/18/16 14:00, Dimitry Sibiryakov wrote:

Specially strange taken together with introduced by you assignment operator using rvalue
reference.

This operator has no advantage on short strings which fit into internal buffer. That's why
control of assignments is still important for performance.

Anyway rollback that "optimization".

AlexPeshkoff · 2016-10-18T11:15:02Z

 			if (dladdr((void*) &allClean, &path))
 			{
 				name = path.dli_fname;
-				name += ".memdebug.log";


Use of function call instead redefined operator does not make code better.
Rollback such changes too.

AlexPeshkoff · 2016-10-18T11:18:59Z

 		T pop()
 		{
 			T* pntr = inherited::pop();
-			T rc = *pntr;


And how is THIS related to unicode support?
May be correct change, but... not here.

aafemt · 2016-10-18T11:19:22Z

Use of function call instead redefined operator does not make code better.

It makes code working. As was agreed in firebird-devel, the function doesn't add directory
separator before appended string.

AlexPeshkoff · 2016-10-18T11:20:33Z

Dmitry, I stop review.
Please remove yourself changes not related to unicode support.
Some of them may be discussed later in separate pull requests.

If you continue playing with names I will have to close your PR which will be very bad cause in many aspects it is useful.

AlexPeshkoff · 2016-10-18T11:28:26Z

On 10/18/16 14:19, Dimitry Sibiryakov wrote:

Use of function call instead redefined operator does not make code better.

It makes code working. As was agreed in firebird-devel, the function doesn't add directory
separator before appended string.

OK, agreed.

AlexPeshkoff · 2016-10-18T12:42:08Z

On 10/18/16 15:31, Dimitry Sibiryakov wrote:

aafemt commented on this pull request.
AbstractString& append(const AbstractString& str)
void append(const AbstractString& str)
These routines are private.

They are protected. Am I undestand it right that
using AbstractString::append;
in public section of derived class makes it visible from the outside world?

They cannot be called from outside code and have nothing to do with public interface of std::basic_string except name.

Class string was initially designed to be able to serve as a replacement
of basic_string with extended functionality and I see no reasons
to change this ability as long as we have no serious reasons to do it.

I.e. to be precise - not only name but name and functionality.

aafemt · 2016-10-18T12:48:40Z

in public section of derived class makes it visible from the outside world?

Yes.

I see no reasons to change this ability as long as we have no serious reasons to do it.

Isn't chained code like "a.append("abc").append("def").c_str()" prohibited by project's style guides?..

AlexPeshkoff · 2016-10-18T12:50:02Z

On 10/18/16 15:48, Dimitry Sibiryakov wrote:

in public section of derived class makes it visible from the outside world?
Yes.

I see no reasons to change this ability as long as we have no serious reasons to do it.
Isn't chained code like "a.append("abc").append("def").c_str()" prohibited by project's style guides?..

May be - but I do not know about it.
Chains of redefined operators are actively used.

aafemt · 2016-10-18T12:55:54Z

Chains of redefined operators are actively used.

But Adriano called them "ugly and non-sense", didn't he?..

AlexPeshkoff · 2016-10-18T12:59:39Z

On 10/18/16 15:55, Dimitry Sibiryakov wrote:

Chains of redefined operators are actively used.
But Adriano called them "ugly and non-sense", didn't he?..

Do not think so - he is actively them. For example, in SQL builder.
But that all goes offtopic here.

AlexPeshkoff · 2016-10-21T14:40:19Z


 	for (burp_fil* file = tdgbl->gbl_sw_files; file; file = file->fil_next)
 	{
-		if (file->fil_name != file_name)


Why this change?
Are we missing operator!=()?

aafemt · 2016-10-25T18:22:29Z

Are we missing operator!=()?

Yes.

dyemanov assigned AlexPeshkoff Apr 19, 2016

aafemt force-pushed the unicode branch from 19ff937 to 7eedbdd Compare May 19, 2016 15:00

aafemt force-pushed the unicode branch from 90546f9 to c885538 Compare June 3, 2016 11:44

aafemt unassigned AlexPeshkoff Jun 6, 2016

aafemt force-pushed the unicode branch from 02fabff to 3915650 Compare June 13, 2016 15:21

AlexPeshkoff reviewed Jun 16, 2016
View reviewed changes

aafemt force-pushed the unicode branch from 5df19a8 to 3d61374 Compare August 9, 2016 15:50

aafemt force-pushed the unicode branch from 6a02269 to 2bed252 Compare August 15, 2016 15:30

Basic support for unicode file names.

88c26d7

String refactoring as requested by Alex.

aafemt force-pushed the unicode branch from 9693fd6 to 88c26d7 Compare September 12, 2016 11:13

aafemt mentioned this pull request Sep 22, 2016

Use restricted list of C++11 features #48

Merged

Partial rollback of string refactoring as requested by Alex

064eca4

AlexPeshkoff reviewed Oct 18, 2016

View reviewed changes

aafemt changed the title ~~Basic support for Unicode file names~~ Basic support for Unicode file names and a lot of subtle bug fixed Oct 18, 2016

More rollback as requested

cf2e8d5

aafemt changed the title ~~Basic support for Unicode file names and a lot of subtle bug fixed~~ Basic support for Unicode file names and some bugfixes Oct 21, 2016

AlexPeshkoff reviewed Oct 21, 2016

View reviewed changes

A line more rollback

3c7043a

aafemt closed this Oct 31, 2016

AlexPeshkoff mentioned this pull request Jan 10, 2023

Attempt to use data in destroyed transaction pool #7446

Closed

AlexPeshkoff mentioned this pull request Mar 14, 2023

Segfault when closing SQL statement in remote provider during shutdown #7504

Closed

lixing-star mentioned this pull request Sep 1, 2023

Add LoongArch support #7531

Merged

hvlad mentioned this pull request Nov 12, 2024

Crash at database restore due to failed system call #8315

Closed

Uh oh!

Conversation

aafemt commented Apr 16, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

egorpugin commented Jul 16, 2016

Uh oh!

hvlad commented Aug 12, 2016

Uh oh!

aafemt commented Aug 12, 2016

Uh oh!

egorpugin commented Aug 12, 2016

Uh oh!

aafemt commented Aug 12, 2016 via email

Uh oh!

egorpugin commented Aug 12, 2016

Uh oh!

aafemt commented Aug 12, 2016 via email

Uh oh!

egorpugin commented Aug 12, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aafemt commented Oct 18, 2016

Uh oh!

aafemt commented Oct 18, 2016

Uh oh!

aafemt commented Oct 18, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aafemt Oct 18, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlexPeshkoff commented Oct 18, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aafemt commented Oct 18, 2016

Uh oh!

AlexPeshkoff commented Oct 18, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlexPeshkoff commented Oct 18, 2016

Uh oh!

AlexPeshkoff commented Oct 18, 2016

Uh oh!

aafemt commented Oct 18, 2016

Uh oh!

AlexPeshkoff commented Oct 18, 2016

Uh oh!

aafemt commented Oct 18, 2016

Uh oh!

AlexPeshkoff commented Oct 18, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aafemt commented Oct 25, 2016 via email

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

aafemt Oct 18, 2016 •

edited

Loading

AlexPeshkoff commented Oct 18, 2016 •

edited

Loading