Здравствуйте, Olegator, Вы писали:

O>Да, это интересно. Но я как раз и хотел, чтобы кто-нибудь помог с оптимизацией моего кода.
Вчера мне было прикольней самому код написать. Типа этюд для программиста.

Главная твоя проблема в развесистом ifе, замаскированном под макрос SELEVENT.
Такие вещи принято заменять на выборку из таблицы:

// Таблица
static int SELEVENT[255];

// Инициализация (можно сделать экономней, особенно если объявить ER = 0, или вообще вбить ручками)

    for (int c = 0 ; c < sizeof(SELEVENT) / sizeof(SELEVENT[0]); ++c)
    {
        if (c == '.') SELEVENT[c] = 6;
        else if (c >= '6' && c <= '9') SELEVENT[c] = 5;
        else if (c == '3' || c == '4') SELEVENT[c] = 3;
        else if (c == '0') SELEVENT[c] = 0;
        else if (c == '1') SELEVENT[c] = 1;
        else if (c == '2') SELEVENT[c] = 2;
        else if (c == '5') SELEVENT[c] = 4;
        else SELEVENT[c] = ER;
    }

// Последний штрих:

        State s = transfer[curstate][SELEVENT[*str]];

И, о чудо! Код работает в два с половиной раза быстрей

O>P.S. Кстати, твоя функция выдаёт true на "022.001.099.003"

Это нормально.

Здравствуйте, Olegator, Вы писали:

O>Во-первых, что сразу бросилось в глаза, твоя функция возвращает true на "000.000.000.000", что не совсем гут.
почему не гуд?
этот адрес является валидным
000.000.000.000 — есть адрес узла, который сгенерировал этот пакет; используется в некоторых сообщениях ICMP.

кстати моя функция ещё пробелы глатает,
то есть такое проглотит

12        .            44        .        4         .       2

но это дело поправимое.

bool IsValidIPAddr(const char* str)
{
    uint_parser<unsigned, 10, 1, 3> uint3_p;
    return parse(str, 
                limit_d(0u, 255u)[uint3_p] >> '.' 
            >>  limit_d(0u, 255u)[uint3_p] >> '.'    
            >>  limit_d(0u, 255u)[uint3_p] >> '.'
            >>  limit_d(0u, 255u)[uint3_p] ).full;    
}

O>Во-вторых, всё же быстродействие страдает. У меня получилось преимущество более, чем в 2 раза.
Согласен.
Да ещё и размер больше.

Но если скорость не критична.
Я бы не заморачивался и выбрал бы мой вариант: просто и красиво, а главное пишется за пол минуты.

Твою функцию я так и не смотрел, извини.
Так что больше ничего сказать по делу не могу.

Вот, собственно, наваял:

#define DIG0(c)  (c == '0')             // 0
#define DIG1(c)  (c == '1')             // 1
#define DIG2(c)  (c == '2')             // 2
#define DIG34(c) (c == '3' || c == '4') // 3
#define DIG5(c)  (c == '5')             // 4
#define DIG69(c) (c >= '6' && c <= '9') // 5
#define DOT(c)   (c == '.')             // 6
#define ER       7                      // 7

#define SELEVENT(c) (DOT(c) ? 6 : (DIG69(c) ? 5 : (DIG34(c) ? 3 : (DIG0(c) ? 0 : (DIG1(c) ? 1 : (DIG2(c) ? 2 : (DIG5(c) ? 4 : ER)))))))

bool IsIPAddr(const char* str)
{
    // x - неизветно, o - известно
    enum State {dig0 = 0, digox, digoo, dig1xx, dig2xx, dig25x, dig1ox, dig2ox, dig25o, dig1oo, dig2oo, dot, er};

    static const State transfer[][8] = {
        {er, er, er, er, er, er, dot, er},
        {digoo, digoo, digoo, digoo, digoo, digoo, dot, er},
        {er, er, er, er, er, er, dot, er},
        {dig1ox, dig1ox, dig1ox, dig1ox, dig1ox, dig1ox, dot, er},
        {dig2ox, dig2ox, dig2ox, dig2ox, dig25x, digoo, dot, er},
        {dig25o, dig25o, dig25o, dig25o, dig25o, er, dot, er},
        {dig1oo, dig1oo, dig1oo, dig1oo, dig1oo, dig1oo, dot, er},
        {dig2oo, dig2oo, dig2oo, dig2oo, dig2oo, dig2oo, dot, er},
        {er, er, er, er, er, er, dot, er},
        {er, er, er, er, er, er, dot, er},
        {er, er, er, er, er, er, dot, er},
        {dig0, dig1xx, dig2xx, digox, digox, digox, er, er},
        {dig0, dig1xx, dig2xx, digox, digox, digox, er, er}
    };

    State curstate = er;
    int dotcount = 0;
    while(*str)
    {
        curstate = transfer[curstate][SELEVENT(*str)];
        if(curstate == er) return false;
        if(curstate == dot) dotcount++;
        if(dotcount > 3) return false;
        str++;
    }

    if(curstate == dot || dotcount < 3) return false;

    return true;
}

Интересует правильность работы функции. Ложный негатив я проверил — все правильные (x.x.x.x, где x число от 0 до 255) адреса выдают true. А как обстоит дело с ложным позитивом?

Также интересует возможность оптимизации функции.

Буду благодарен за помощь.

С уважением,
Olegator

Здравствуйте, Olegator, Вы писали:

O>Также интересует возможность оптимизации функции.

Блин, проглючил! Сам нашёл лишний кусок. Лучше так:

    enum State {dig0 = 0, digox, dig1xx, dig2xx, dig25x, dig1ox, dig2ox, digdone, dot, er};

    static const State transfer[][8] = {
        {er, er, er, er, er, er, dot, er},
        {digdone, digdone, digdone, digdone, digdone, digdone, dot, er},
        {dig1ox, dig1ox, dig1ox, dig1ox, dig1ox, dig1ox, dot, er},
        {dig2ox, dig2ox, dig2ox, dig2ox, dig25x, digdone, dot, er},
        {digdone, digdone, digdone, digdone, digdone, er, dot, er},
        {digdone, digdone, digdone, digdone, digdone, digdone, dot, er},
        {digdone, digdone, digdone, digdone, digdone, digdone, dot, er},
        {er, er, er, er, er, er, dot, er},
        {dig0, dig1xx, dig2xx, digox, digox, digox, er, er},
        {dig0, dig1xx, dig2xx, digox, digox, digox, er, er}
    };

Тем не менее, вопрос всё ещё в силе.

С уважением,
Olegator

Здравствуйте, Olegator, Вы писали:

O>Блин, проглючил! Сам нашёл лишний кусок. Лучше так:

Да, туплю я не по-детски:

    enum State {dig1xx, dig2xx, dig25x, digoox, digdone, dot, er};

    static const State transfer[][8] = {
        {digoox, digoox, digoox, digoox, digoox, digoox, dot, er},
        {digoox, digoox, digoox, digoox, dig25x, digdone, dot, er},
        {digdone, digdone, digdone, digdone, digdone, er, dot, er},
        {digdone, digdone, digdone, digdone, digdone, digdone, dot, er},
        {er, er, er, er, er, er, dot, er},
        {digdone, dig1xx, dig2xx, digoox, digoox, digoox, er, er},
        {digdone, dig1xx, dig2xx, digoox, digoox, digoox, er, er}
    };

Почему-то ошибки мне легче наблюдать, когда я вижу их на форуме RSDN. Может есть ещё что-нибудь?

С уважением,
Olegator

Здравствуйте, Olegator, Вы писали:

Я конечно дико извиняюсь что влезаю немножко не по теме.
Но вот сел читать про boost/spirit.
Сижу вот полчасика уже читаю.
Ну и наваял такой маленький примерчик:

#include <boost/spirit/core.hpp>
using namespace boost::spirit;

bool IsValidIPAddr(const char* str)
{
    uint_parser<unsigned, 10, 1, 3> uint2_p;
    return parse(str,
        //  Begin grammar
        (
                limit_d(0u, 255u)[uint2_p] >> '.'
            >>  limit_d(0u, 255u)[uint2_p] >> '.'    
            >>  limit_d(0u, 255u)[uint2_p] >> '.'
            >>  limit_d(0u, 255u)[uint2_p]
        )
        ,
        //  End grammar
        space_p).full;    
}

Не знаю как там по эффективности, но за то просто и красиво
А твой код не смотрел потому что пока времени нет.

Здравствуйте, korzhik, Вы писали:

K>Я конечно дико извиняюсь что влезаю немножко не по теме.
K>Но вот сел читать про boost/spirit.
K>Сижу вот полчасика уже читаю.

Во-первых, что сразу бросилось в глаза, твоя функция возвращает true на "000.000.000.000", что не совсем гут.

Во-вторых, всё же быстродействие страдает. Посмотри:

    const int num = 1000000;
    char** buf = new char*[num];
    srand(GetTickCount());
    for(int i = 0; i < num; i++)
    {
        buf[i] = new char[16];
        sprintf(buf[i], "%i.%i.%i.%i", rand() % 257, rand() % 256, rand() % 258, rand() % 256);
    }

    DWORD yourstart, yourend, mystart, myend;
    int yourwrongs = 0, mywrongs = 0;

    yourstart = GetTickCount();
    for(int i = 0; i < num; i++)
        if(!IsValidIPAddr(buf[i]))
            yourwrongs++;
    yourend = GetTickCount();

    mystart = GetTickCount();
    for(int i = 0; i < num; i++)
        if(!IsIPAddr(buf[i]))
            mywrongs++;
    myend = GetTickCount();

    cout << "Results: " << endl
         << "Your time: " << yourend - yourstart << endl
         << "Your wrongs: " << yourwrongs << endl << endl
         << "My time: " << myend - mystart << endl
         << "My wrongs: " << mywrongs << endl << endl;

    for(int i = 0; i < num; i++)
        delete[] buf[i];
    delete[] buf;

У меня получилось преимущество более, чем в 2 раза.

С уважением,
Olegator

Может по-простому, циферка за циферкой? В полтора раза быстрее работает, судя по твоим же тестам. VC 7.1 Release, Optimize speed.

bool isValidIP(const char* ip)
{
    if (!ip)
        return false;

    for (int j = 0 ; j < 4 ; ++j)
    {
        int i = 0;
        while (i < 3 && ip[i] && ip[i] != '.')
            ++i;

        char limit = '9';
        switch(i)
        {
        case 3:
            if (*ip < 0 || *ip > '2')
                return false;
            if (*ip == '2')
                limit = '5';
            ++ip;
        case 2:
            if (*ip < 0 || *ip > limit)
                return false;
            if (*ip < limit)
                limit = '9';
            ++ip;
        case 1:
            if (*ip < 0 || *ip > limit)
                return false;
            ++ip;
            break;
        case 0:
            return false;
        }
        if (j < 3 && *ip++ != '.')
            return false;
    }
    return *ip == '\0';
}

Здравствуйте, Olegator.

Вы писали 18 апреля 2004 г., 12:22:37:

O> Интересует правильность работы функции. Ложный негатив я
O> проверил — все правильные (x.x.x.x, где x число от 0 до 255) адреса
O> выдают true. А как обстоит дело с ложным позитивом?

IP адрес может быть представлен не только в формате x.x.x.x
пример:
адрес 10.0.0.32 = 10.32 = 10.0.32 = 167772192 = 0x0A000020
в случае 0.0.0.0 — это тоже адрес = INADDR_ANY

--
С уважением, butcher

Здравствуйте, <Аноним>, Вы писали:

А>Может по-простому, циферка за циферкой? В полтора раза быстрее работает, судя по твоим же тестам. VC 7.1 Release, Optimize speed.

Да, это интересно. Но я как раз и хотел, чтобы кто-нибудь помог с оптимизацией моего кода.

P.S. Кстати, твоя функция выдаёт true на "022.001.099.003"

С уважением,
Olegator

Вот здесь 256, конечно:
D>static int SELEVENT[256];

Вот тут в оригинале должно быть
        curstate = transfer[curstate][SELEVENT[*str]];

D>        State s = transfer[curstate][SELEVENT[*str]];

Здравствуйте, degor, Вы писали:

D>Главная твоя проблема в развесистом ifе, замаскированном под макрос SELEVENT.
D>Такие вещи принято заменять на выборку из таблицы:

Вот за это — огромное спасибо!

С уважением,
Olegator

Здравствуйте, Olegator, Вы писали:

O>Вот, собственно, наваял:

// skip

Может я чего не понимаю, но чем вот это вариант не устраивает?

  struct in_addr ip1;
  ip1.S_un.S_addr = inet_addr(edStartIP->Text.c_str());

  if (ip1.S_un.S_addr == INADDR_NONE)
  {
    MessageBox(Handle, "Start IP field does not contain a legitimate Internet address", "ins/error", MB_ICONERROR);
  }

Здравствуйте, BlackBox, Вы писали:

BB>Может я чего не понимаю, но чем вот это вариант не устраивает?

А зачем искать легкие пути ?

BB>

BB>  ip1.S_un.S_addr = inet_addr(edStartIP->Text.c_str());
BB>