Ticket #157 (closed enhancement: fixed)

Opened 8 years ago

Last modified 7 years ago

[PATCH] Micro helper library

Reported by: Enrico.Weigelt@…> Owned by: metux
Priority: critical Milestone: 4.6.2
Component: mc-core Version: 4.6.1
Keywords: vote-winnie vote-slavazanko committed-master committed-mc-4.6 Cc:
Blocked By: Blocking: #10, #14, #125, #147, #152, #179
Branch state: Votes for changeset:

Description (last modified by metux) (diff)

this patch introduces some new mhl/*.h files with things like (safer) memory management and string handling, shell escaping, etc

Committed to master:
changeset:4765514421d29983040a1d6a51789c7d4cdf4342

Committed to mc-4.6:
changeset:1e2ed2f808c79b101d2ca70d3f25a3db41eb77b9

Attachments

mhl.001.diff (16.5 KB) - added by anonymous 8 years ago.
Added by email2trac
mhl.002.diff (13.8 KB) - added by slavazanko 8 years ago.
extend mhl: use glib (if #defined WITH_GLIB)
mhl.from001-to002.diff (3.6 KB) - added by slavazanko 8 years ago.
Changes from rev001 to rev002 between patches for better review
own_inline.patch (2.8 KB) - added by slavazanko 8 years ago.
Some checks for GNU-extensions in compiler for support inline'ing

Change History

Changed 8 years ago by anonymous

Added by email2trac

comment:1 Changed 8 years ago by anonymous

  • id set to 157

This message has 1 attachment(s)

comment:2 Changed 8 years ago by metux

  • Keywords review added
  • Type changed from defect to enhancement
  • Description modified (diff)
  • Blocking 10, 14, 41, 55, 81, 125, 147, 149, 152 added

comment:3 in reply to: ↑ description Changed 8 years ago by andrew_b

Replying to Enrico.Weigelt@…>:

this patch introduces some new mhl/*.h

IMHO, 'mhl' name is not intuitively :)

comment:4 Changed 8 years ago by metux

  • Status changed from new to accepted
  • Owner set to metux

comment:5 Changed 8 years ago by Enrico Weigelt

  • MC Ticket System <tickets@…> schrieb:

    IMHO, 'mhl' name is not intuitively :)

you have a better idea ? ;-p

comment:6 Changed 8 years ago by andrew_b

  • Keywords rework added; review removed

Type corrections are needed: strlen() returns a value of type size_t, not int.

comment:7 Changed 8 years ago by slavazanko

What about $(src_root)/lib directory?

Why is it necessary to create a directory 'mhl', if there is a directory 'lib'?

My propose:

git-mv lib contrib
git-mv mhl lib
git-commit

comment:8 Changed 8 years ago by slavazanko

in addition to my previous comment:

... and, of course, change files configure.ac and Makefile.am for correctly compile with new changes.

comment:9 Changed 8 years ago by winnie

Well.. as this should go to 4.6.2 which is only a bugfixing release I doesn't want to restructure the code here.

We can restructurize the code afterwards in a new ticket ( I think there is already a ticket about restructurizing the code)... This would then belong into this ticket not into this one.

I would now (for the 4.6.X branch) leave mhl where it is (or move it to lib/mhl but leaving the rest as it is.

comment:10 Changed 8 years ago by slavazanko

Ah... if after release-4.6 out mhl will move to lib - I agree :)

Changed 8 years ago by slavazanko

extend mhl: use glib (if #defined WITH_GLIB)

Changed 8 years ago by slavazanko

Changes from rev001 to rev002 between patches for better review

comment:11 Changed 8 years ago by slavazanko

oops...

mhl/memory.h:4

#if defined _AIX && !defined REGEX_MALLOC 
#pragma alloca 
#endif 

REGEX_MALLOC must be removed. This stuff copyed from src/regex.c (for correctly declate 'alloca' function).

In any case, patch need to review and rework.

comment:12 Changed 8 years ago by metux

@slava: your alloca() changes are very fine, but:

a) the glib stuff is quite useless, just adds extra code without any benefit and makes the binary more expensive (adds farcalls to funcs which far-call funcs that could have been inlined)

b) moving the funcs to separate .c file just kills inline'ing.

comment:13 Changed 8 years ago by winnie

  • Milestone changed from 4.7 to 4.6.2

Setting milestone to 4.6.2 as we noone on the ML said anything against this and we really need parts of this patch in order to fix other important stuff.

comment:14 follow-up: ↓ 18 Changed 8 years ago by slavazanko

a) the glib stuff is quite useless, just adds extra code

On a modern computers it's don't important :) For other systems we make own realization of glib-functions... if needed :)

b) moving the funcs to separate .c file just kills inline'ing.

Modern compilers may automatically make some function inline or not. In mc-ru-fork we was attempt to use own standart _GNU_INLINE_ (make some checks in configure-script)

See next attach.

Some example of usage:

_GNU_INLINE_ void _ATTRIBUTE_ALWAYS_INLINE_
some_function_1(params){
   ...
}
static _GNU_INLINE_ char *
some_function_2(params){
   ...
}
...

For more info about this need to consult with Pavlinux(Russian team of mc-ru-fork)... but he not present in this trac... :(

P.S. I will return inline'ing to functions back. :)

Changed 8 years ago by slavazanko

Some checks for GNU-extensions in compiler for support inline'ing

comment:15 Changed 8 years ago by slavazanko

  • Priority changed from major to critical

comment:16 Changed 8 years ago by slavazanko

BTW, in mhl/string.c some warnings:

string.c: In function ‘mhl_str_reverse’:
string.c:38: warning: implicit declaration of function ‘strlen’
string.c:38: warning: incompatible implicit declaration of built-in function ‘strlen’
...

adding string to a top of mhl/string.c:

#include <string.h>

don't avoid warnings. Because one of current includedir in CFLAGS is a: -I./
I think, need to rename mhl/string.[ch] to mhl/mhl_string.[ch]
This avoid warnings. See branch; changeset:51a81ec6e3191a693d09a20e337056055bf1daef

Or may exists other way?

comment:17 Changed 8 years ago by winnie

  • Keywords review vote-winnie added; rework removed

Hey,

After a discussion with slavaz on jabber I created a new branch called 157_mhl_additions_to_master which the stuff recently added by him. After that I've reverted the stuff from slavaz in the old branch so that we can get a minimalistic stuff into 4.6.2 to fix there some problems.

So please have _now_ a look on the 157_mhl_addition branch and vote! :)

comment:18 in reply to: ↑ 14 Changed 8 years ago by ossi

Replying to slavazanko:

b) moving the funcs to separate .c file just kills inline'ing.

Modern compilers may automatically make some function inline or not.

uhm, so what? moving to a separate .c file still kills inlining. period.

comment:19 Changed 8 years ago by Enrico Weigelt

  • MC Ticket System <tickets@…> schrieb:

Comment(by slavazanko):

a) the glib stuff is quite useless, just adds extra code

On a modern computers it's don't important :)

Useless code is always a mess, at least for maintenance.
I'm really curios which problem you intend to solve - or is it
just glib-fetishism ? ;-o

For other systems we make own realization of glib-functions... if needed :)

We dont need any extra solution, just leave it as it was.

b) moving the funcs to separate .c file just kills inline'ing.

Modern compilers may automatically make some function inline or not.

Only if they can. As soon as you put the code in an extra .c file, which
gets compiled separately, the compiler has no chance to do inline'ing.

Some example of usage:

_GNU_INLINE_ void _ATTRIBUTE_ALWAYS_INLINE_
some_function_1(params){
   ...
}

This would only make sense if you want to *enforce* inline'ing on
an per-function basis. Is this really necessary ? Do you really feel
more clever than the compiler on whether to inline certain specific
function ?

cu

comment:20 Changed 8 years ago by slavazanko

  • Keywords vote-slavazanko added

comment:21 Changed 8 years ago by winnie

  • Keywords approved added

Cool.. this is approved now.

@metux: Could you please merge this into mc-4.6 and master? After that I'll work on the whitespace issues and completion. :)

comment:22 Changed 8 years ago by slavazanko

  • Keywords review removed

comment:23 Changed 8 years ago by slavazanko

  • Blocking 179 added

comment:24 Changed 8 years ago by metux

  • Keywords committed-master committed-mc-4.6 added; approved removed
  • Status changed from accepted to testing
  • Resolution set to fixed
  • Description modified (diff)

comment:25 Changed 8 years ago by winnie

  • Status changed from testing to closed

Closing. Remote branch is removed.

comment:26 Changed 7 years ago by andrew_b

  • Blocking 81 removed

comment:27 Changed 6 years ago by andrew_b

  • Blocking 55 removed

comment:28 Changed 6 years ago by andrew_b

  • Blocking 41 removed

comment:29 Changed 6 years ago by andrew_b

  • Blocking 149 removed
Note: See TracTickets for help on using tickets.